Bug 82670

Summary:

First step toward incremental Weak<T> finalization

Product:

WebKit

Reporter:

Geoffrey Garen <ggaren>

Component:

New Bugs

Assignee:

Geoffrey Garen <ggaren>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fpizlo, gustavo, mhahnenberg, pnormand, rakuco, sam, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

83139

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	fpizlo: review+

Geoffrey Garen

Reported 2012-03-29 15:25:52 PDT

First step toward incremental Weak<T> finalization

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
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Geoffrey Garen

Comment 1 2012-03-29 16:23:27 PDT

Created attachment 134688 [details] Patch

Philippe Normand

Comment 2 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

Build Bot

Comment 3 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

Build Bot

Comment 4 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

Early Warning System Bot

Comment 5 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

Early Warning System Bot

Comment 6 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

Gyuyoung Kim

Comment 7 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

Geoffrey Garen

Comment 8 2012-03-29 19:47:16 PDT

Created attachment 134716 [details] Patch

Gustavo Noronha (kov)

Comment 9 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

Early Warning System Bot

Comment 10 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

Early Warning System Bot

Comment 11 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

Build Bot

Comment 12 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

Build Bot

Comment 13 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

Gyuyoung Kim

Comment 14 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

Geoffrey Garen

Comment 15 2012-03-30 10:55:11 PDT

Created attachment 134838 [details] Patch

Early Warning System Bot

Comment 16 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

Early Warning System Bot

Comment 17 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

Build Bot

Comment 18 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

Build Bot

Comment 19 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

Geoffrey Garen

Comment 20 2012-03-30 11:38:52 PDT

Created attachment 134848 [details] Patch

Philippe Normand

Comment 21 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

Early Warning System Bot

Comment 22 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

Build Bot

Comment 23 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

Early Warning System Bot

Comment 24 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

Geoffrey Garen

Comment 25 2012-03-30 15:10:13 PDT

Created attachment 134896 [details] Patch

Philippe Normand

Comment 26 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

Build Bot

Comment 27 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

Early Warning System Bot

Comment 28 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

Geoffrey Garen

Comment 29 2012-03-31 18:18:39 PDT

Created attachment 134982 [details] Patch

Build Bot

Comment 30 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

Philippe Normand

Comment 31 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

Gustavo Noronha (kov)

Comment 32 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

Build Bot

Comment 33 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

Geoffrey Garen

Comment 34 2012-04-02 09:04:35 PDT

Created attachment 135114 [details] Patch

Philippe Normand

Comment 35 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

Early Warning System Bot

Comment 36 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

Geoffrey Garen

Comment 37 2012-04-02 14:59:02 PDT

Created attachment 135199 [details] Patch

Gustavo Noronha (kov)

Comment 38 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

Geoffrey Garen

Comment 39 2012-04-02 16:15:16 PDT

Created attachment 135219 [details] Patch

WebKit Review Bot

Comment 40 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.

Filip Pizlo

Comment 41 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() ?

Oliver Hunt

Comment 42 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?

Geoffrey Garen

Comment 43 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.

Geoffrey Garen

Comment 44 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.

Geoffrey Garen

Comment 45 2012-04-03 22:28:22 PDT

Committed r113141: <http://trac.webkit.org/changeset/113141>

Geoffrey Garen

Comment 46 2012-04-04 00:18:26 PDT

Follow-up fix for WebKit2: <http://trac.webkit.org/changeset/113146>.

Geoffrey Garen

Comment 47 2012-04-04 10:17:56 PDT

Follow-up fix for Qt / 32-bit: <http://trac.webkit.org/changeset/113209>.

Note You need to log in before you can comment on or make changes to this bug.