Closed Bug 1127156 Opened 5 years ago Closed 5 years ago

Expose JIT optimization information to TableTicker

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(4 files, 5 obsolete files)

Once bug 1030389 lands, it needs to be hooked up to the TableTicker so we can actually expose the optimization info.

Interface
---------

I plan to add an additional property 'ionOpts' (in addition to 'location', 'line', and 'category') to each frame.

This property will be initially something like:

  ionOpts :: [ types, attempts ]

  types :: type | type, types
  type :: { site: str, mirType: int }

  attempts :: attempt | attempt, attempts
  attempt :: { strategy: str, outcome: int }

Currently I plan to add at least constructor:lineno info to the 'type' production above, for exposing TypeObjects.

Implementation
--------------

Both the types and attempts lists defined above are already stored in a native code-addressed side table in JS land. That is, there's a side table mapping some address to a vector.

To save precious circular buffer space, I plan to add a new ProfileEntry tag 'j' that stores an index into this side table.

Symbolication (i.e., reading out the types and attempts) is then done during JSON streaming time. This seems pretty threadsafe to me, this side table isn't a fancy data structure and isn't self-modifying during reads.
Depends on: 1030389
So, as I tried to hook up this stuff to the profiler, I realized I didn't
design the internals with public JSAPI in mind.

This refactors some stuff, makes the enums public, and reworks the public API.
Attachment #8557046 - Flags: review?(kvijayan)
Comment on attachment 8557046 [details] [diff] [review]
Rework optimization tracking JSAPI to be more usable from the profiler.

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

Cancelling review. I'm still iterating.
Attachment #8557046 - Flags: review?(kvijayan)
Attachment #8557046 - Attachment is obsolete: true
Attachment #8558291 - Flags: review?(kvijayan)
Attachment #8557047 - Attachment is obsolete: true
Attachment #8558293 - Flags: feedback?(bgirard)
Comment on attachment 8558293 [details] [diff] [review]
Attach optimization info to frames in profile.

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

::: tools/profiler/ProfileEntry.cpp
@@ +95,5 @@
>    // and union variant fields, so the following was derived
>    // by looking through all the use points of TableTicker.cpp.
>    //   mTagMarker (ProfilerMarker*) m
>    //   mTagData   (const char*)  c,s
> +  //   mTagPtr    (void*)        d,l,L,E,B (immediate backtrace), S(start-of-stack)

Please add the name/payload description in parentheses for what the enum type should be. I'm not sure what 'E' stands for.

::: tools/profiler/TableTicker.cpp
@@ +586,5 @@
> +      // Symbolication of optimization information is delayed until streaming
> +      // time. To re-lookup the entry in the JitcodeGlobalTable, we need to
> +      // store both the the return address ('E') and the tracked optimization
> +      // index ('o') in the circular buffer.
> +      if (jsFrames[jsIndex].hasTrackedOptimizations) {

Nice. This will have a trivial runtime overhead. I'm assuming the js code also adds minimal runtime overhead? If not consider making it an optional feature. Otherwise this is great!
Attachment #8558293 - Flags: feedback?(bgirard) → feedback+
Comment on attachment 8558291 [details] [diff] [review]
Rework optimization tracking JSAPI to be more usable from the profiler.

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

::: js/public/TrackedOptimizationInfo.h
@@ +278,5 @@
> +JS_PUBLIC_API(void)
> +ForEachTrackedOptimizationAttempt(JSRuntime *rt, void *addr, uint8_t index,
> +                                  ForEachTrackedOptimizationAttemptOp &op);
> +
> +struct ForEachTrackedOptimizationTypeInfoOp

For this type, could you document what the expected protocol is for sequences of calls to "operator()" vs "readType" that the interface can expect?

::: js/src/jit/OptimizationTracking.h
@@ +426,5 @@
> +    enum {
> +        HasNothing,
> +        HasAllocationSite,
> +        HasConstructor
> +    } hasAddendum;

Ew.  Give this a named type Addendum or something.
Attachment #8558291 - Flags: review?(kvijayan) → review+
Renamed 'E' to 'J' for JIT code address for clarity.

This will be landed disabled at the JS level. That is, I will land this but no
JS frames will have an optimization info index.
Attachment #8558293 - Attachment is obsolete: true
Attachment #8558774 - Flags: review?(bgirard)
Comment on attachment 8558774 [details] [diff] [review]
Attach optimization info to frames in profile.

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

Err, hold on, I screwed up rebasing.
Attachment #8558774 - Flags: review?(bgirard)
Rebased across bhackett's ObjectGroup rename. Carrying r=djvj.
Attachment #8558291 - Attachment is obsolete: true
Attachment #8558783 - Flags: review+
Attachment #8558774 - Attachment is obsolete: true
Attachment #8558784 - Flags: review?(bgirard)
Attachment #8558784 - Flags: review?(bgirard) → review+
Blocks: 1129232
This is burning inbound.
https://treeherder.mozilla.org/logviewer.html#?job_id=6239375&repo=mozilla-inbound
Assignee: nobody → shu
Flags: needinfo?(shu)
Flags: needinfo?(shu)
Blocks: 1129510
https://hg.mozilla.org/mozilla-central/rev/39422c6d5efc
https://hg.mozilla.org/mozilla-central/rev/dd1cdbf4be40
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1130698
You need to log in before you can comment on or make changes to this bug.