Bug 212878 - Lazily allocate HistoricUsageData
Summary: Lazily allocate HistoricUsageData
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-07 02:05 PDT by Yusuke Suzuki
Modified: 2021-12-19 13:11 PST (History)
4 users (show)

See Also:


Attachments
Patch (1.57 KB, patch)
2020-06-07 02:06 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (1.77 KB, patch)
2020-06-07 02:07 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-06-07 02:05:48 PDT
Lazily allocate HistoricUsageData
Comment 1 Yusuke Suzuki 2020-06-07 02:06:23 PDT
Created attachment 401289 [details]
Patch
Comment 2 Yusuke Suzuki 2020-06-07 02:07:32 PDT
Created attachment 401290 [details]
Patch
Comment 3 Sam Weinig 2020-06-07 08:21:41 PDT
Hey Yusuke, just so I understand a little better, what is the benefit of removing this from the __DATA section?
Comment 4 Saam Barati 2020-06-08 12:58:10 PDT
(In reply to Sam Weinig from comment #3)
> Hey Yusuke, just so I understand a little better, what is the benefit of
> removing this from the __DATA section?

I believe the goal is to reduce dirty memory usage
Comment 5 Saam Barati 2020-06-08 12:59:40 PDT
(In reply to Saam Barati from comment #4)
> (In reply to Sam Weinig from comment #3)
> > Hey Yusuke, just so I understand a little better, what is the benefit of
> > removing this from the __DATA section?
> 
> I believe the goal is to reduce dirty memory usage

I guess to elaborate on how I model my understanding:
This will take away space from other things in DATA that could’ve theoretically fit on one page and perhaps bumping it into two.
Comment 6 Yusuke Suzuki 2020-06-08 13:02:45 PDT
(In reply to Saam Barati from comment #5)
> (In reply to Saam Barati from comment #4)
> > (In reply to Sam Weinig from comment #3)
> > > Hey Yusuke, just so I understand a little better, what is the benefit of
> > > removing this from the __DATA section?
> > 
> > I believe the goal is to reduce dirty memory usage
> 
> I guess to elaborate on how I model my understanding:
> This will take away space from other things in DATA that could’ve
> theoretically fit on one page and perhaps bumping it into two.

Yes, since __DATA is per page, it is possible that we construct a page like,


[<---------------------------historicUsageData-------------><-small data->]

In that case, if small data gets dirty, it taints entire page dirty even if historicUsageData part is not used at all.
Comment 7 Sam Weinig 2020-06-08 13:18:28 PDT
(In reply to Yusuke Suzuki from comment #6)
> (In reply to Saam Barati from comment #5)
> > (In reply to Saam Barati from comment #4)
> > > (In reply to Sam Weinig from comment #3)
> > > > Hey Yusuke, just so I understand a little better, what is the benefit of
> > > > removing this from the __DATA section?
> > > 
> > > I believe the goal is to reduce dirty memory usage
> > 
> > I guess to elaborate on how I model my understanding:
> > This will take away space from other things in DATA that could’ve
> > theoretically fit on one page and perhaps bumping it into two.
> 
> Yes, since __DATA is per page, it is possible that we construct a page like,
> 
> 
> [<---------------------------historicUsageData-------------><-small data->]
> 
> In that case, if small data gets dirty, it taints entire page dirty even if
> historicUsageData part is not used at all.

Interesting. Would it ever make sense to annotate these rarely used large data portions with a specific section name to allow the linker to group rarely used data together?
Comment 8 Yusuke Suzuki 2021-12-19 01:35:44 PST
(In reply to Sam Weinig from comment #7)
> (In reply to Yusuke Suzuki from comment #6)
> > (In reply to Saam Barati from comment #5)
> > > (In reply to Saam Barati from comment #4)
> > > > (In reply to Sam Weinig from comment #3)
> > > > > Hey Yusuke, just so I understand a little better, what is the benefit of
> > > > > removing this from the __DATA section?
> > > > 
> > > > I believe the goal is to reduce dirty memory usage
> > > 
> > > I guess to elaborate on how I model my understanding:
> > > This will take away space from other things in DATA that could’ve
> > > theoretically fit on one page and perhaps bumping it into two.
> > 
> > Yes, since __DATA is per page, it is possible that we construct a page like,
> > 
> > 
> > [<---------------------------historicUsageData-------------><-small data->]
> > 
> > In that case, if small data gets dirty, it taints entire page dirty even if
> > historicUsageData part is not used at all.
> 
> Interesting. Would it ever make sense to annotate these rarely used large
> data portions with a specific section name to allow the linker to group
> rarely used data together?

It is nice! So, if it is the data we need definitely, we should do that. But at the same time, by doing heap-allocation here, this patch reduces binary size :)
Comment 9 Yusuke Suzuki 2021-12-19 01:41:03 PST
Committed r287238 (245398@trunk): <https://commits.webkit.org/245398@trunk>
Comment 10 Radar WebKit Bug Importer 2021-12-19 01:42:19 PST
<rdar://problem/86680604>
Comment 11 Simon Fraser (smfr) 2021-12-19 09:31:31 PST
Funny, I did this same thing but found that it didn't change binary size.

The __DATA/_bss section is constructed at runtime (fresh pages mapped into memory). But if it has the side effect of shuffling around other things in __DATA then maybe it can affect binary size?
Comment 12 Simon Fraser (smfr) 2021-12-19 09:33:30 PST
See bug 233476.
Comment 13 Yusuke Suzuki 2021-12-19 13:11:52 PST
(In reply to Simon Fraser (smfr) from comment #11)
> Funny, I did this same thing but found that it didn't change binary size.
> 
> The __DATA/_bss section is constructed at runtime (fresh pages mapped into
> memory). But if it has the side effect of shuffling around other things in
> __DATA then maybe it can affect binary size?

Ah! Yes. But anyway, the original intent of this patch is avoid having dirty __DATA memory by purging large rarely used data from __DATA (as described in https://bugs.webkit.org/show_bug.cgi?id=212878#c6), and it is still held :)