[webkit-reviews] review denied: [Bug 65901] The JSC JIT currently has no facility to profile and report the types of values : [Attachment 103335] the patch (fix style and build)

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Filip Pizlo
<fpizlo at apple.com>'s request for review:
Bug 65901: The JSC JIT currently has no facility to profile and report the
types of values
https://bugs.webkit.org/show_bug.cgi?id=65901

Attachment 103335: the patch (fix style and build)
https://bugs.webkit.org/attachment.cgi?id=103335&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=103335&action=review


r- for a bunch of small suggested changes, mostly just that ValueProfile really
should be in its own header.

> Source/JavaScriptCore/bytecode/CodeBlock.h:207
> +#if ENABLE(VALUE_PROFILER)

The file CodeBlock is a bit too big already, and this struct is non-trivial, I
think we really need to add a new ValueProfile.h header.

> Source/JavaScriptCore/bytecode/CodeBlock.h:682
> +	   SegmentedVector<ValueProfile, sizeof(ValueProfile) * 8>
m_valueProfiles;

This code is a little weird - I think the second argument to the template
should be a count of ValueProfiles, not a size, so isn't this growing n^2 in
the size of ValueProfile? - don't you just want 8 here?

> Source/JavaScriptCore/jit/JIT.cpp:377
> +    unsigned numberOfValueProfiles = m_codeBlock->numberOfValueProfiles();

Is this just asserting that no slow cases add any new (FirstProfilingSite)
value profiling code? - I think an comment here would be good.

> Source/JavaScriptCore/jit/JIT.h:313
> +#endif

Are value / scratch ever passed? - I didn't see any case of this.
If not, it may make the code simpler to remove these arguments.
Also, this function may trample regT2/regT3 - I think it's worth commenting
this on the method, to try to make this more apparent.