Bug 61024 – Use placement new to allocate statics in BSS

• Status: NEW
• Component: Web Template Framework

[Pratik Solanki]

Update DEFINE_STATIC_LOCAL to allocate statics in BSS instead of the program heap since these objects never get freed.

[Pratik Solanki]

Created attachment 93876 [details]
Patch

[Pratik Solanki]

Props to Anders for suggesting this placement new trick of coalescing these static objects in the BSS.

[Andreas Kling]

Comment on attachment 93876 [details]
Patch

Cool idea! r=me

[Alexey Proskuryakov]

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.

[Andreas Kling]

(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.

[Alexey Proskuryakov]

> 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).

[Andreas Kling]

(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*)".

[Alexey Proskuryakov]

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.

[Anders Carlsson]

(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.

[Pratik Solanki]

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.

[Alexey Proskuryakov]

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.

[Anders Carlsson]

(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.

[Alexey Proskuryakov]

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.