Bug 82670 - First step toward incremental Weak<T> finalization
Summary: First step toward incremental Weak<T> finalization
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords:
Depends on: 83139
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-29 15:25 PDT by Geoffrey Garen
Modified: 2012-04-04 10:17 PDT (History)
8 users (show)

See Also:


Attachments
Patch (92.81 KB, patch)
2012-03-29 16:23 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (92.85 KB, patch)
2012-03-29 19:47 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (92.87 KB, patch)
2012-03-30 10:55 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (100.14 KB, patch)
2012-03-30 11:38 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (100.76 KB, patch)
2012-03-30 15:10 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (101.28 KB, patch)
2012-03-31 18:18 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (101.78 KB, patch)
2012-04-02 09:04 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (102.53 KB, patch)
2012-04-02 14:59 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff
Patch (101.79 KB, patch)
2012-04-02 16:15 PDT, Geoffrey Garen
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2012-03-29 15:25:52 PDT
First step toward incremental Weak<T> finalization
Comment 1 Geoffrey Garen 2012-03-29 16:23:27 PDT
Created attachment 134688 [details]
Patch
Comment 2 Philippe Normand 2012-03-29 16:34:16 PDT
Comment on attachment 134688 [details]
Patch

Attachment 134688 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12240002
Comment 3 Build Bot 2012-03-29 16:44:09 PDT
Comment on attachment 134688 [details]
Patch

Attachment 134688 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12248004
Comment 4 Build Bot 2012-03-29 16:46:20 PDT
Comment on attachment 134688 [details]
Patch

Attachment 134688 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12248001
Comment 5 Early Warning System Bot 2012-03-29 16:56:57 PDT
Comment on attachment 134688 [details]
Patch

Attachment 134688 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12249003
Comment 6 Early Warning System Bot 2012-03-29 17:07:46 PDT
Comment on attachment 134688 [details]
Patch

Attachment 134688 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12255002
Comment 7 Gyuyoung Kim 2012-03-29 18:23:04 PDT
Comment on attachment 134688 [details]
Patch

Attachment 134688 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12267005
Comment 8 Geoffrey Garen 2012-03-29 19:47:16 PDT
Created attachment 134716 [details]
Patch
Comment 9 Gustavo Noronha (kov) 2012-03-29 19:55:21 PDT
Comment on attachment 134716 [details]
Patch

Attachment 134716 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12266035
Comment 10 Early Warning System Bot 2012-03-29 20:05:27 PDT
Comment on attachment 134716 [details]
Patch

Attachment 134716 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12291006
Comment 11 Early Warning System Bot 2012-03-29 20:05:52 PDT
Comment on attachment 134716 [details]
Patch

Attachment 134716 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12266040
Comment 12 Build Bot 2012-03-29 20:13:06 PDT
Comment on attachment 134716 [details]
Patch

Attachment 134716 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12265025
Comment 13 Build Bot 2012-03-29 20:38:12 PDT
Comment on attachment 134716 [details]
Patch

Attachment 134716 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12266034
Comment 14 Gyuyoung Kim 2012-03-29 21:30:49 PDT
Comment on attachment 134716 [details]
Patch

Attachment 134716 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12265049
Comment 15 Geoffrey Garen 2012-03-30 10:55:11 PDT
Created attachment 134838 [details]
Patch
Comment 16 Early Warning System Bot 2012-03-30 11:07:47 PDT
Comment on attachment 134838 [details]
Patch

Attachment 134838 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12266406
Comment 17 Early Warning System Bot 2012-03-30 11:12:04 PDT
Comment on attachment 134838 [details]
Patch

Attachment 134838 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12265352
Comment 18 Build Bot 2012-03-30 11:19:21 PDT
Comment on attachment 134838 [details]
Patch

Attachment 134838 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12265351
Comment 19 Build Bot 2012-03-30 11:24:32 PDT
Comment on attachment 134838 [details]
Patch

Attachment 134838 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12267337
Comment 20 Geoffrey Garen 2012-03-30 11:38:52 PDT
Created attachment 134848 [details]
Patch
Comment 21 Philippe Normand 2012-03-30 12:06:30 PDT
Comment on attachment 134848 [details]
Patch

Attachment 134848 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12288352
Comment 22 Early Warning System Bot 2012-03-30 12:11:40 PDT
Comment on attachment 134848 [details]
Patch

Attachment 134848 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12253385
Comment 23 Build Bot 2012-03-30 12:16:23 PDT
Comment on attachment 134848 [details]
Patch

Attachment 134848 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12253378
Comment 24 Early Warning System Bot 2012-03-30 12:20:34 PDT
Comment on attachment 134848 [details]
Patch

Attachment 134848 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12288357
Comment 25 Geoffrey Garen 2012-03-30 15:10:13 PDT
Created attachment 134896 [details]
Patch
Comment 26 Philippe Normand 2012-03-30 15:42:49 PDT
Comment on attachment 134896 [details]
Patch

Attachment 134896 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12288470
Comment 27 Build Bot 2012-03-30 15:44:10 PDT
Comment on attachment 134896 [details]
Patch

Attachment 134896 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12253490
Comment 28 Early Warning System Bot 2012-03-30 16:07:29 PDT
Comment on attachment 134896 [details]
Patch

Attachment 134896 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12291472
Comment 29 Geoffrey Garen 2012-03-31 18:18:39 PDT
Created attachment 134982 [details]
Patch
Comment 30 Build Bot 2012-03-31 19:23:50 PDT
Comment on attachment 134982 [details]
Patch

Attachment 134982 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12291937
Comment 31 Philippe Normand 2012-03-31 19:26:49 PDT
Comment on attachment 134982 [details]
Patch

Attachment 134982 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12308021
Comment 32 Gustavo Noronha (kov) 2012-03-31 19:30:17 PDT
Comment on attachment 134982 [details]
Patch

Attachment 134982 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12288940
Comment 33 Build Bot 2012-03-31 20:02:02 PDT
Comment on attachment 134982 [details]
Patch

Attachment 134982 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12253990
Comment 34 Geoffrey Garen 2012-04-02 09:04:35 PDT
Created attachment 135114 [details]
Patch
Comment 35 Philippe Normand 2012-04-02 09:32:00 PDT
Comment on attachment 135114 [details]
Patch

Attachment 135114 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12308642
Comment 36 Early Warning System Bot 2012-04-02 12:09:52 PDT
Comment on attachment 135114 [details]
Patch

Attachment 135114 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12312645
Comment 37 Geoffrey Garen 2012-04-02 14:59:02 PDT
Created attachment 135199 [details]
Patch
Comment 38 Gustavo Noronha (kov) 2012-04-02 16:03:05 PDT
Comment on attachment 135199 [details]
Patch

Attachment 135199 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12306917
Comment 39 Geoffrey Garen 2012-04-02 16:15:16 PDT
Created attachment 135219 [details]
Patch
Comment 40 WebKit Review Bot 2012-04-02 16:20:00 PDT
Attachment 135219 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1
Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Filip Pizlo 2012-04-03 13:18:30 PDT
Comment on attachment 135219 [details]
Patch

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

R=me except for the one comment about possibly not having to do a round of tracing after handling dead weak handles.

> Source/JavaScriptCore/heap/Heap.cpp:716
> +        {
> +            ParallelModeEnabler enabler(visitor);
> +            visitor.donateAndDrain();
> +#if ENABLE(PARALLEL_GC)
> +            visitor.drainFromShared(SlotVisitor::MasterDrain);
> +#endif
> +        }

Why do we have to retrace?  I don't see marking in visitDeadWeakImpls.

> Source/JavaScriptCore/heap/Heap.h:138
> +        WeakHeap* weakHeap() { return &m_weakHeap; }

Random nit: whenever I look at this code I get confused by the terms "WeakHeap" and "HandleHeap".  Those aren't really heaps.  I mean, they do some of their own memory management, but so do many things in our code, and yet we don't call them "heaps" for that reason.

I feel like in the future - not in this patch - it might be good to rename "HandleHeap" and "WeakHeap" to "HandleSet" and "WeakSet".  Thoughts?

> Source/JavaScriptCore/heap/WeakHeap.h:73
> +    if (!allocator)

UNLIKELY() ?
Comment 42 Oliver Hunt 2012-04-03 13:22:02 PDT
Comment on attachment 135219 [details]
Patch

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

> Source/JavaScriptCore/Configurations/Base.xcconfig:-27
> -COMPILER_SPECIFIC_WARNING_CFLAGS_LLVM_COMPILER = -Wglobal-constructors -Wexit-time-destructors;

Why have you removed this?
Comment 43 Geoffrey Garen 2012-04-03 15:16:05 PDT
> > -COMPILER_SPECIFIC_WARNING_CFLAGS_LLVM_COMPILER = -Wglobal-constructors -Wexit-time-destructors;
> 
> Why have you removed this?

Accident. I was testing with GCC to try to figure out the build failures. Will fix before landing.
Comment 44 Geoffrey Garen 2012-04-03 15:20:52 PDT
> Why do we have to retrace?  I don't see marking in visitDeadWeakImpls.

True, we don't yet. I'll remove this, and add it back once it makes sense.

> > Source/JavaScriptCore/heap/Heap.h:138
> > +        WeakHeap* weakHeap() { return &m_weakHeap; }
> 
> Random nit: whenever I look at this code I get confused by the terms "WeakHeap" and "HandleHeap".  Those aren't really heaps.  I mean, they do some of their own memory management, but so do many things in our code, and yet we don't call them "heaps" for that reason.
> 
> I feel like in the future - not in this patch - it might be good to rename "HandleHeap" and "WeakHeap" to "HandleSet" and "WeakSet".  Thoughts?

Sounds OK. If we go with "WeakSet", I might want to change "allocate" and "deallocate" to "add" and "remove". I'm a little worried that "*Set" implies uniqueness, which is not true in this case. Maybe "WeakList"?

> > Source/JavaScriptCore/heap/WeakHeap.h:73
> > +    if (!allocator)
> 
> UNLIKELY() ?

Fixed.
Comment 45 Geoffrey Garen 2012-04-03 22:28:22 PDT
Committed r113141: <http://trac.webkit.org/changeset/113141>
Comment 46 Geoffrey Garen 2012-04-04 00:18:26 PDT
Follow-up fix for WebKit2: <http://trac.webkit.org/changeset/113146>.
Comment 47 Geoffrey Garen 2012-04-04 10:17:56 PDT
Follow-up fix for Qt / 32-bit: <http://trac.webkit.org/changeset/113209>.