Bug 66747

Summary: There is no facility for profiling how the write barrier is used
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: darin, fpizlo, ggaren, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
the patch
the patch (fix conflict with my earlier patch)
webkit-ews: commit-queue-
the patch (fix added file)
ggaren: review-
the patch (fix review)
ggaren: review+
the patch (fix review)
fpizlo: commit-queue-
the patch (fix style) none

Description Filip Pizlo 2011-08-22 23:28:54 PDT
Write barrier support was added to the JSC runtime in anticipation of the implementation of more sophisticated garbage collection schemes, such as possibly generational or incremental collection.  Write barrier fast paths can often be optimized for specific kinds of writes.  In some collectors, a store to a location that is in the heap can be optimized far more aggressively than a store to a location allocated outside the heap.  One example of this is the range-based generational barrier, which uses the heap address of the destination object as a quick check to see if the barrier needs to be run.

But designing such barriers requires knowing what kinds of writes programs tend to execute.  Currently, there is no way to tell the barrier what kind of write is being executed.  And there is no way to profile and report the kinds of writes encountered.
Comment 1 Filip Pizlo 2011-08-23 00:26:12 PDT
Created attachment 104796 [details]
the patch
Comment 2 Filip Pizlo 2011-08-23 00:40:04 PDT
Created attachment 104797 [details]
the patch (fix conflict with my earlier patch)
Comment 3 Early Warning System Bot 2011-08-23 00:52:52 PDT
Comment on attachment 104797 [details]
the patch (fix conflict with my earlier patch)

Attachment 104797 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9475286
Comment 4 Filip Pizlo 2011-08-23 01:11:21 PDT
Created attachment 104801 [details]
the patch (fix added file)

In the previous patch, I forgot to include a file I had added.
Comment 5 Geoffrey Garen 2011-08-23 11:20:33 PDT
Comment on attachment 104801 [details]
the patch (fix added file)

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

Looks good, but I think the C++ counter is not quite right.

> Source/JavaScriptCore/runtime/WriteBarrier.h:151
>      // when some basic types aren't yet completely instantiated
>      void setEarlyValue(JSGlobalData&, const JSCell* owner, T* value)
>      {
> +        WriteBarrierCounters::usesWithBarrierFromCpp.count();
> +#endif
>          this->m_cell = reinterpret_cast<JSCell*>(value);
>          Heap::writeBarrier(owner, this->m_cell);

I think you want to put the counter inside Heap::writeBarrier, and not setEarlyValue. Heap::writeBarrier is the central funnel that will catch all C++ access.
Comment 6 Filip Pizlo 2011-08-23 13:04:49 PDT
Created attachment 104902 [details]
the patch (fix review)
Comment 7 Geoffrey Garen 2011-08-23 16:00:13 PDT
Comment on attachment 104902 [details]
the patch (fix review)

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

r=me, but you have a minor bug here in the ENABLE(GGC) case to fix.

> Source/JavaScriptCore/heap/Heap.h:253
>      inline void Heap::writeBarrier(const JSCell* owner, JSValue value)
>      {
> +        WriteBarrierCounters::countWriteBarrier();
>          if (!value)
>              return;
>          if (!value.isCell())

Since this version of Heap::writeBarrier calls through to the JSCell* version, you'll double-count cases where you don't early return. You need to call WriteBarrierCounters::countWriteBarrier() inside each early return, and not in the body of the function.
Comment 8 Filip Pizlo 2011-08-24 00:49:29 PDT
Created attachment 104969 [details]
the patch (fix review)
Comment 9 WebKit Review Bot 2011-08-24 00:51:07 PDT
Attachment 104969 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/heap/Heap.h:135:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 22 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Filip Pizlo 2011-08-24 00:58:36 PDT
Comment on attachment 104969 [details]
the patch (fix review)

Oops, apparently the fix breaks style.
Comment 11 Filip Pizlo 2011-08-24 01:04:41 PDT
Created attachment 104970 [details]
the patch (fix style)
Comment 12 WebKit Review Bot 2011-08-24 02:50:55 PDT
Comment on attachment 104970 [details]
the patch (fix style)

Clearing flags on attachment: 104970

Committed r93698: <http://trac.webkit.org/changeset/93698>
Comment 13 WebKit Review Bot 2011-08-24 02:51:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Darin Adler 2011-08-24 09:45:21 PDT
I think that a better nickname for “calls from C++ as opposed to from JavaScript” would be FromC rather than FromCpp. I find it hard to parse Cpp since it’s not capitalized as an acronym and when used as an acronym it often is used to mean “C preprocessor”. I think that C is a fine nickname for “the C family including C++” in this context.