Bug 66747 – There is no facility for profiling how the write barrier is used

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 104796 [details]
the patch

[Filip Pizlo]

Created attachment 104797 [details]
the patch (fix conflict with my earlier patch)

[Early Warning System Bot]

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

[Filip Pizlo]

Created attachment 104801 [details]
the patch (fix added file)

In the previous patch, I forgot to include a file I had added.

[Geoffrey Garen]

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)
>      {
> +#if ENABLE(WRITE_BARRIER_PROFILING)
> +        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.

[Filip Pizlo]

Created attachment 104902 [details]
the patch (fix review)

[Geoffrey Garen]

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.

[Filip Pizlo]

Created attachment 104969 [details]
the patch (fix review)

[WebKit Review Bot]

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.

[Filip Pizlo]

Comment on attachment 104969 [details]
the patch (fix review)

Oops, apparently the fix breaks style.

[Filip Pizlo]

Created attachment 104970 [details]
the patch (fix style)

[WebKit Review Bot]

Comment on attachment 104970 [details]
the patch (fix style)

Clearing flags on attachment: 104970

Committed r93698: <http://trac.webkit.org/changeset/93698>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.