Summary: | There is no facility for profiling how the write barrier is used | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | darin, fpizlo, ggaren, oliver, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2011-08-22 23:28:54 PDT
Created attachment 104796 [details]
the patch
Created attachment 104797 [details]
the patch (fix conflict with my earlier patch)
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 Created attachment 104801 [details]
the patch (fix added file)
In the previous patch, I forgot to include a file I had added.
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. Created attachment 104902 [details]
the patch (fix review)
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. Created attachment 104969 [details]
the patch (fix review)
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 on attachment 104969 [details]
the patch (fix review)
Oops, apparently the fix breaks style.
Created attachment 104970 [details]
the patch (fix style)
Comment on attachment 104970 [details] the patch (fix style) Clearing flags on attachment: 104970 Committed r93698: <http://trac.webkit.org/changeset/93698> All reviewed patches have been landed. Closing bug. 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. |