Annotate binary arith for jit coach

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 years ago
Created attachment 8711722 [details] [diff] [review]
bugXXX-jitcoach-info
Assignee: nobody → hv1989
Attachment #8711722 - Flags: review?(shu)

Comment 2

2 years ago
Comment on attachment 8711722 [details] [diff] [review]
bugXXX-jitcoach-info

Review of attachment 8711722 [details] [diff] [review]:
-----------------------------------------------------------------

Cool! Thanks for annotating.

Docs for the tracked optimizations are kept in-tree in js/src/doc/JITOptimizations/. As a followup, could you document the new enums you added?
Attachment #8711722 - Flags: review?(shu) → review+
(Assignee)

Comment 3

2 years ago
Created attachment 8714705 [details] [diff] [review]
bug1242578-jitcoach-info-part2
Attachment #8714705 - Flags: review?(shu)

Comment 4

2 years ago
Comment on attachment 8714705 [details] [diff] [review]
bug1242578-jitcoach-info-part2

Review of attachment 8714705 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits addressed. Thanks for documenting.

::: js/src/doc/JITOptimizations/Strategies.md
@@ +28,5 @@
> +## <a name="binaryarith"></a>BinaryArith
> +
> +### BinaryArith_Concat
> +
> +Attempts to optimize an addition to string concatenation. The types of the operands are checked to contain a hint of being a concatenation. Both have to be string or one has to be a string and the other a type that easily can get converted to string (i.e. numeric).

I think 'e.g.' is more appropriate here.

@@ +40,5 @@
> +Just like BinrayArith_SpecializedTypes tries to do a numeric arithmetic, but based on the observed types in the Baseline Compiler. If it succeeds it will roughly give same optimization as BinaryArith_SpecializedTypes, but the deduction is more fragile. In the best case one should try to get this optimization based on the input types.
> +
> +### BinaryArith_SharedCache
> +
> +Attempts to optimize a binary arithmetic by using stubs. The stubs itself are type specialized, but the instruction itself is a black box. As a result the operation can still get optimized based on the input types, but no global optimization can happen.

I think this provides more information than is necessary and isn't actionable information for the JS dev anyways. The gist here is that these are inline caches, right? I'd just say this is the inline cache case.

@@ +44,5 @@
> +Attempts to optimize a binary arithmetic by using stubs. The stubs itself are type specialized, but the instruction itself is a black box. As a result the operation can still get optimized based on the input types, but no global optimization can happen.
> +
> +### BinaryArith_Call
> +
> +Last resort which cannot get specialized at all. This calls into C++ code to do this operation.

No need to mention calling into C++. Saying it's the slow path suffices.
Attachment #8714705 - Flags: review?(shu) → review+

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/53630278e423
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb724661e17

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/53630278e423
https://hg.mozilla.org/mozilla-central/rev/3bb724661e17
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1246154
(Assignee)

Comment 7

2 years ago
Created attachment 8716327 [details] [diff] [review]
bug1246154-tracksuccess

I just noticed all other code has "trackOptimizationSuccess", except for the code I added. Is it needed?
Attachment #8716327 - Flags: feedback?(shu)

Comment 8

2 years ago
Comment on attachment 8716327 [details] [diff] [review]
bug1246154-tracksuccess

Review of attachment 8716327 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, I missed this, good catch.

You do need trackOptimizationSuccess(), yeah. When you start tracking a particular strategy, the default outcome is GenericFailure. Successful optimization requires explicit tracking.
Attachment #8716327 - Flags: feedback?(shu) → feedback+
(Assignee)

Comment 9

2 years ago
Comment on attachment 8716327 [details] [diff] [review]
bug1246154-tracksuccess

Review of attachment 8716327 [details] [diff] [review]:
-----------------------------------------------------------------

Requesting r?, before I can push.
Attachment #8716327 - Flags: review?(shu)

Updated

2 years ago
Attachment #8716327 - Flags: review?(shu) → review+

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/89065d2e4220

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89065d2e4220
You need to log in before you can comment on or make changes to this bug.