Bug 61024 - Use placement new to allocate statics in BSS
Summary: Use placement new to allocate statics in BSS
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-18 00:03 PDT by Pratik Solanki
Modified: 2013-02-22 11:58 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.45 KB, patch)
2011-05-18 00:09 PDT, Pratik Solanki
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2011-05-18 00:03:24 PDT
Update DEFINE_STATIC_LOCAL to allocate statics in BSS instead of the program heap since these objects never get freed.
Comment 1 Pratik Solanki 2011-05-18 00:09:25 PDT
Created attachment 93876 [details]
Patch
Comment 2 Pratik Solanki 2011-05-18 00:09:46 PDT
Props to Anders for suggesting this placement new trick of coalescing these static objects in the BSS.
Comment 3 Andreas Kling 2011-05-18 04:41:20 PDT
Comment on attachment 93876 [details]
Patch

Cool idea! r=me
Comment 4 Alexey Proskuryakov 2011-05-18 10:34:05 PDT
Comment on attachment 93876 [details]
Patch

I'm not sure if this is sufficiently safe. Nothing guarantees that size of an object matches sizeof - the class may override operator new to allocate a different amount of space. We used to do that with some strings to avoid buffer allocation.

Besides, objects that are allocated this way won't work with fastMallocSize, potentially triggering assertion failures.

r-, because this at least needs a more detailed risk/benefit analysis. I suspect that downsides outweigh the benefits.
Comment 5 Andreas Kling 2011-05-18 11:03:47 PDT
(In reply to comment #4)
> (From update of attachment 93876 [details])
> I'm not sure if this is sufficiently safe. Nothing guarantees that size of an object matches sizeof - the class may override operator new to allocate a different amount of space. We used to do that with some strings to avoid buffer allocation.

I couldn't find any instances of that (overridden operator new + DEFINE_STATIC_LOCAL) in WebKit, so I believed this was safe.

> Besides, objects that are allocated this way won't work with fastMallocSize, potentially triggering assertion failures.

But of course, you are completely right about this. Thanks for correcting.
Comment 6 Alexey Proskuryakov 2011-05-18 11:15:25 PDT
> I couldn't find any instances of that (overridden operator new + DEFINE_STATIC_LOCAL) in WebKit, so I believed this was safe.

I do not know of a way to detect this situation to catch it early. WebKit code tends to evolve rapidly.

> But of course, you are completely right about this. Thanks for correcting.

Anders suggested that to avoid tripping over assertions, we could limit this to release builds. This may be fine in practice, although it does make the platform weaker (fastMallocSize will be unreliable in release builds).
Comment 7 Andreas Kling 2011-05-18 11:57:51 PDT
(In reply to comment #6)
> > I couldn't find any instances of that (overridden operator new + DEFINE_STATIC_LOCAL) in WebKit, so I believed this was safe.
> 
> I do not know of a way to detect this situation to catch it early. WebKit code tends to evolve rapidly.

AFAIK, if "operator new(size_t)" is the only override, then it should be easy to catch since placement new won't compile without "operator new(size_t, void*)".
Comment 8 Alexey Proskuryakov 2011-05-18 12:16:11 PDT
Pratik is going to see if this patch provides significant benefits. Most statically initialized objects also allocate multiple objects in heap, so storing the "main" object alone in BSS may not have any practical effect.
Comment 9 Anders Carlsson 2011-05-18 12:38:36 PDT
(In reply to comment #8)
> Pratik is going to see if this patch provides significant benefits. Most statically initialized objects also allocate multiple objects in heap, so storing the "main" object alone in BSS may not have any practical effect.

I'd like us to be able to enable DEFINE_STATIC_LOCAL to store even more data in BSS, such as the StringImpl, and also store the string data in read-only memory so that it can be shared between processes.
Comment 10 Pratik Solanki 2011-05-18 13:49:52 PDT
Yes, I'll investigate this some more. I think my big concern now is to see if any of the static objects allocated allocate more memory in their constructors which would end up being on the heap.

(In reply to comment #7)
> AFAIK, if "operator new(size_t)" is the only override, then it should be easy to catch since placement new won't compile without "operator new(size_t, void*)".

Good point! This should eliminate one of Alexey's concerns.(In reply to comment #9)

(In reply to comment #9)
> I'd like us to be able to enable DEFINE_STATIC_LOCAL to store even more data in BSS, such as the StringImpl, and also store the string data in read-only memory so that it can be shared between processes.

Yeah. I can see if this is doable and if it gives us any benefits.
Comment 11 Alexey Proskuryakov 2013-02-22 11:30:49 PST
One issue with using more static storage is that you always pay the cost for it, whether a particular feature is used or not. This is because pages are become dirty if only a single variable gets changed.
Comment 12 Anders Carlsson 2013-02-22 11:53:16 PST
(In reply to comment #11)
> One issue with using more static storage is that you always pay the cost for it, whether a particular feature is used or not. This is because pages are become dirty if only a single variable gets changed.

Right. That’s why we should use order files to group them together.
Comment 13 Alexey Proskuryakov 2013-02-22 11:58:13 PST
Order files can help with not incurring the cost for unused features (but it's profile based, so only helps if usage scenario matches one used for profiling). But heap allocations are even better in this case - if a feature is unused, there is no allocation and no fragmentation.

I think that we should be very careful with the idea of replacing heap fragmentation with DATA fragmentation.