Bug 116472 – Add WTF::NeverDestroyed and start using it in WTF

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Anders Carlsson]

Add WTF::NeverDestroyed and start using it in WTF

[Anders Carlsson]

Created attachment 202330 [details]
Patch

[WebKit Commit Bot]

Attachment 202330 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmakefile.list.am', u'Source/WTF/WTF.vcproj/WTF.vcproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.xcodeproj/project.pbxproj', u'Source/WTF/wtf/Compiler.h', u'Source/WTF/wtf/CryptographicallyRandomNumber.cpp', u'Source/WTF/wtf/NeverDestroyed.h', u'Source/WTF/wtf/Noncopyable.h', u'Source/WTF/wtf/text/WTFString.cpp']" exit_code: 1
Source/WTF/wtf/NeverDestroyed.h:53:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/NeverDestroyed.h:74:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 202330 [details]
Patch

I love it!

[Anders Carlsson]

Committed r150450: <http://trac.webkit.org/changeset/150450>

[Darin Adler]

Comment on attachment 202330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202330&action=review

> Source/WTF/ChangeLog:10
> +        NeverDestroyed is a class template that can be used for singletons and other objects that we never
> +        want to destroy. It's intended as a replacement for WTF_STATIC_LOCAL with the advantage that it doesn't
> +        fragment the heap.

Are we going to change DEFINE_STATIC_LOCAL to use this, at least as an initial step? Or just globally replace DEFINE_STATIC_LOCAL with this and delete DEFINE_STATIC_LOCAL?

[Alexey Proskuryakov]

> Are we going to change DEFINE_STATIC_LOCAL to use this, at least as an initial step? Or just globally replace DEFINE_STATIC_LOCAL with this and delete DEFINE_STATIC_LOCAL?

I guess it's blocked on the FIXME, as fragmenting the DATA segment is pretty bad too, maybe even worse than fragmenting the heap.

+    // FIXME: Investigate whether we should allocate a hunk of virtual memory
+    // and hand out chunks of it to NeverDestroyed instead, to reduce fragmentation.

[Anders Carlsson]

(In reply to comment #6)
> > Are we going to change DEFINE_STATIC_LOCAL to use this, at least as an initial step? Or just globally replace DEFINE_STATIC_LOCAL with this and delete DEFINE_STATIC_LOCAL?
> 
> I guess it's blocked on the FIXME, as fragmenting the DATA segment is pretty bad too, maybe even worse than fragmenting the heap.

I don’t think this is true. The current size of the __bss section in WebCore is 30k and I expect the fastMalloc heap to be much bigger than this.

[Alexey Proskuryakov]

> I don’t think this is true. The current size of the __bss section in WebCore is 30k and I expect the fastMalloc heap to be much bigger than this.

I'm not sure how this addresses the concern.

1. The current size is not the right metric, since DEFINE_STATIC_LOCAL just stores pointers, and NeverDestroyed will increase the size quite a bit.

2. Total size is not a good metric. Heap can be large, but that does not mean that we suffer from its fragmentation enough to make such a change a win.

The downside of growing static data is that unlike with heap, empty space can never be used for something else, it's statically allocated.

3. Why only measure WebCore, not the other frameworks?

4. There is no data suggesting that switching to NeverDestroyed will improve heap fragmentation. With the notable exception of emptyString, objects tend to have heap-allocated fields.

One thing I don't know is what exactly goes to __bss. We have noticeably more dirty memory in __DATA/__data. What's that?

[Darin Adler]

I’m not sure that “fragmenting” is the real issue here. Here are the memory pros and cons that I know of:

Allocation on the heap
- Memory cost: malloc overhead in addition to the object size
- Memory cost: pointer in global data in addition to the object size
- Speed cost: must dereference a pointer to get at the object
- Speed cost: must page in part of the heap to get at the object

Allocation in a global
- Memory cost: object size memory is always allocated even for code paths that are never used
- Speed cost: more memory to zero when initializing the library
- Speed cost: less locality between other globals since these globals are larger, so possibly more paging to access globals

I personally think we should drop the heap-based solution.

[Alexey Proskuryakov]

> - Memory cost: object size memory is always allocated

This is almost the same what I'm saying, but not quite the same. This memory is not only allocated, but it's also copied on write and dirtied. A single static boolean that's modified will dirty 4K of otherwise unused memory reserved for some never-used feature if we get unlucky.

There are some ways to reorder the data segment, but they are of course only helping as long as the scenarios we used to record usage match actual user scenarios.

[Benjamin Poulain]

> There are some ways to reorder the data segment, but they are of course only helping as long as the scenarios we used to record usage match actual user scenarios.

Personally, this is my main concern.

I think static allocation on the heap introduces too many problems, which is why I like NeverDestroyed. But I would like all the startup statics on the same pages or we may dirty pages all over the place.

Another option is to allocate a dense memory zone just for statics. This is more work for us but solve fragmenting in both cases (and probably help with locality).

[Darin Adler]

I think that most of these static locals are not for never-used features. The use of the heap for these was simply a hack to avoid the destructor, not an attempt to reduce the cost when the function is never called. It seems unfortunate we got attached to possible benefits of the accidental side effect of using heap allocation.

[Anders Carlsson]

Ignoring heap vs non-heap, I vastly prefer the class template syntax over the macro and that's really my mai reason for wanting to switch. If we find out that using the data segment is worse than the heap we can always change the NeverDestroyed implementation.