Closed Bug 1145442 Opened 5 years ago Closed 4 years ago

Documentation for JIT strategies

Categories

(Developer Documentation :: Developer Tools, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jsantell, Assigned: djvj)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [leave open])

Attachments

(3 files, 5 obsolete files)

If we use JIT strategy constant names as a unique identifier for optimization strategies, we can use this to link to additional information regarding what strategy failed. On the front end we can have the raw strategy attempts link to the docs, which could just be one or two simple sentences -- this is more of a reference, not suggestions on how to write fast code.

Not sure if we should link to MDN/wiki.moz/wherever there is JIT documentation, or maybe have it open up a new page `about:jitopts#GetProp_IC` or whatever. As this is mainly for JIT/SM engineers at the moment, and is documentation, I don't think this needs to be localized.
I think MDN is the best plan here.  That way, there's a public resource that's accessible outside of the tool as well.

How do I go about creating a new MDN page, and what path should I use for it?
Flags: needinfo?(jsantell)
What exactly will be documented? Optimizations? Data types? Would this be devtools specific, or apply to any use of our JITs? Wondering if it should live under js or devtools on mdn.

Pinging Will if there's any sort of format that should be used, although I suspect we'll need a slightly different parser than the CSS docs (will look more how we display those later today)
Flags: needinfo?(jsantell) → needinfo?(wbamberg)
It think it's best if it lives under devtools for now on mdn.  If at some point there's a special 'jit' MDN category, it might fit under there.  But these are specific optimization strategy details relevant to a particular engine.  It relates to either engine details specifically, or devtools, not with "javascript" as a concept.

The first thing to document is optimizations.

So for example, somebody sees that a GetProp_InferredConstant failure showed up at an optimization site, they can go to a wiki page that tells them that this is an optimization that operates on "singleton" objects, and tries to infer that a value on this object is a constant on the basis that it hasn't been assigned to more than once.  They can learn how the optimization is defeated, and what kind of situations it's useful for.

Likewise for all the GetProp, SetProp, GetElem, and SetElem variants.
What should the format of each entry be?

Kannan proposes 4 sections:

1. What it optimizes
2. How it tries to optimize
3. How can it fail
4. Common fixes

We should definitely write out in more detail for common opts (GetProp_DefiniteSlot). What of the rarely applicable ones, how do we compress the 4 sections above, as there are a myriad of ways they can fail.

Sections 1, 3, and 4 could all benefit greatly from concrete examples.
A comprehensive "How it can fail" might grow very large very quickly.

I suppose the format might look more like this:

For each Strategy:

1. What it tries to optimize
2. How it can fail (list of links to Outcomes)
3. How it can succeed (list of links to Outcomes)

For each failure Outcome:

1. How it can fail
2. Common fixes

For each success Outcome:

1. Common patterns that succeed
That seems reasonable.  It does risk the potential of forcing the reader to go to different pages, which breaks their flow.  If they visit an optimization strategy, they should be able to get a pretty complete description of what they need right there.  Maybe not all the nitty gritty details, but a good overall picture of the optimization.

What if in the 'how it can fail' and 'how it can succeed' sections, in addition to a link to the list of outcomes (with detailed info), we present a blurb (small paragraph) summarizing the broad failure and success conditions?
Also, here's a copypaste of the list of current opt strategies. We should divvy these up:

Here's a copy-paste of the current set optimization strategies from OptimizationTrackingInfo.h.  We should just divvy this up somehow and knock 'em off one by one:

GetProp_ArgumentsLength
GetProp_ArgumentsCallee
GetProp_InferredConstant
GetProp_Constant
GetProp_StaticName
GetProp_SimdGetter
GetProp_TypedObject
GetProp_DefiniteSlot
GetProp_Unboxed
GetProp_CommonGetter
GetProp_InlineAccess
GetProp_Innerize
GetProp_InlineCache

SetProp_CommonSetter
SetProp_TypedObject
SetProp_DefiniteSlot
SetProp_Unboxed
SetProp_InlineAccess
SetProp_InlineCache

GetElem_TypedObject
GetElem_Dense
GetElem_TypedStatic
GetElem_TypedArray
GetElem_String
GetElem_Arguments
GetElem_ArgumentsInlined
GetElem_InlineCache

SetElem_TypedObject
SetElem_TypedStatic
SetElem_TypedArray
SetElem_Dense
SetElem_Arguments
SetElem_InlineCache

Call_Inline
(In reply to Kannan Vijayan [:djvj] from comment #6)
> That seems reasonable.  It does risk the potential of forcing the reader to
> go to different pages, which breaks their flow.  If they visit an
> optimization strategy, they should be able to get a pretty complete
> description of what they need right there.  Maybe not all the nitty gritty
> details, but a good overall picture of the optimization.
> 
> What if in the 'how it can fail' and 'how it can succeed' sections, in
> addition to a link to the list of outcomes (with detailed info), we present
> a blurb (small paragraph) summarizing the broad failure and success
> conditions?

This sounds fine to me.

(In reply to Kannan Vijayan [:djvj] from comment #7)
> Also, here's a copypaste of the list of current opt strategies. We should
> divvy these up:
> 
> Here's a copy-paste of the current set optimization strategies from
> OptimizationTrackingInfo.h.  We should just divvy this up somehow and knock
> 'em off one by one:
> 

I just had one of them manager-type delegation ideas. WHAT IF, we held a documentation "party" with other JIT folks at Whistler and write some of this over drinks? Might be a good opportunity for each other to get a broader picture of the "macro" optimizations our JIT does.
Talked to Will about this, relaying here:

* The docs should live under Spidermonkey[0] docs somewhere, as these are all specific to Spidermonkey (and would be helpful on its own for a SM hacker), and nothing is "devtools" specific, as its just documenting SM's enums.

* The format doesn't matter for using tooltips in the tools -- only thing that matters is that they're consistent, as they get parsed for sections. For example, when parsing the CSS docs with the raw version[1] of MDN, only the summary and syntax sections are displayed. Each piece of documentation that is to be linked must also be anchor-linked, so we can just fetch a section by id[2].

I'm not sure what kind of docs should be listed here, w/r/t how it can be optimized, how it can fail/be fixed, etc., but whatever you think would be appropriate to anyone interested in SM. Some thoughts are we should probably only show a moderately short summary in the tooltip (with a link to MDN for more), and  while this is still geared towards JIT/SM hackers and still pretty inaccessible, maybe the "common fixes" docs can be the groundwork for a "JIT Coach" that can display tips in the call tree, or something.

Will offered to do editorial reviews and the like for any docs here, as well as any discussions in Whistler about this.

[0] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey
[1] https://developer.mozilla.org/en-US/docs/Web/CSS/background-color?raw&macros
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/margin?raw&macros&section=Syntax
Flags: needinfo?(wbamberg)
Looks like there'll be a docs jam later in the week at Whistler, if you want to overlap there, I'd imagine there'd be a few people from MDN to help out on any formatting and editorial questions.

via Jeremie Patonnier:
> From Basically, on Thursday and Friday the MDN team will take over the Algonqin room at the Fairmont
> Hotel and write documentation like the crazy foxes we are.
I wrote up a skeleton MDN page here.  Let me know what you guys think:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JIT_Optimization_Strategies
Flags: needinfo?(shu)
Flags: needinfo?(jsantell)
Looks great so far -- is it possible to have one complete example of what info would be there? Can see how easy it'd be to link from the tools then
Flags: needinfo?(jsantell)
Seconded. Looks good. I'll start writing some stuff about inlining.
Flags: needinfo?(shu)
I started moving the docs in tree with some minor edits. Not done copying
everything yet.
Attachment #8632424 - Flags: feedback?(kvijayan)
Attached patch jitcoach-strategy-docs.patch (obsolete) — Splinter Review
Reposing the markdown documentation patch.  Copied all the documentation from the wiki.  Filled in a couple more.  Let's check this in and work on it in pieces.
Attachment #8632424 - Attachment is obsolete: true
Attachment #8632424 - Flags: feedback?(kvijayan)
Attachment #8640683 - Flags: review?(shu)
Attached patch jitcoach-strategy-docs.patch (obsolete) — Splinter Review
Updated patch to include documentation for all getprops.

I'm questioning the need to add docs for the optimization outcomes directly to this document.  I think your earlier idea of keeping those in a separate place is a better idea.

It's a LOT of work to track down and document each of these on a per-optimization-strategy basis (and it's slowing down the strategy documentation, which is arguably more important than detailing the outcomes).

I think we should remove the outcomes entirely and move them to a different .md file.

Anyway, for this review just check over the explanations and check them for sanity and language.
Attachment #8640683 - Attachment is obsolete: true
Attachment #8640683 - Flags: review?(shu)
Attachment #8640765 - Flags: review?(shu)
(In reply to Kannan Vijayan [:djvj] from comment #16)
> Created attachment 8640765 [details] [diff] [review]
> jitcoach-strategy-docs.patch
> 
> Updated patch to include documentation for all getprops.
> 
> I'm questioning the need to add docs for the optimization outcomes directly
> to this document.  I think your earlier idea of keeping those in a separate
> place is a better idea.
> 
> It's a LOT of work to track down and document each of these on a
> per-optimization-strategy basis (and it's slowing down the strategy
> documentation, which is arguably more important than detailing the outcomes).
> 
> I think we should remove the outcomes entirely and move them to a different
> .md file.

Sounds good to me. Put another way, the strategies documentation only documents the success outcome, and we move the failure outcomes to another file.
Comment on attachment 8640765 [details] [diff] [review]
jitcoach-strategy-docs.patch

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

Thanks for copying stuff over.

::: js/src/doc/JITOptimizations/config.sh
@@ +1,3 @@
> +base-url https://developer.mozilla.org/en-US/docs/Tools/
> +
> +markdown Strategies.md JIT-Optimization-Strategies

Since we're moving outcomes to a separate file, this should probably be JIT-Optimization/Strategies instead of JIT-Optimization-Strategies
Attachment #8640765 - Flags: review?(shu) → review+
Attached patch jitcoach-strategy-docs.patch (obsolete) — Splinter Review
Updated strategy docs to include descriptions for all optimization strategies.  Not the most full-fledged descriptions for all of them, but it gets _something_ in there which can be iterated on with time.
Attachment #8640765 - Attachment is obsolete: true
Attachment #8641115 - Flags: review?(shu)
Attached patch jitcoach-strategy-docs.patch (obsolete) — Splinter Review
Sorry for repeatedly doing this.  Last time I promise.

Split the strategies and outcomes into different .md files.  Fixed config file.  Removed outcome documentation from strategies file.

I'll push this after r+.  No more changes.  Promise.
Attachment #8641115 - Attachment is obsolete: true
Attachment #8641115 - Flags: review?(shu)
Attachment #8641177 - Flags: review?(shu)
Comment on attachment 8641177 [details] [diff] [review]
jitcoach-strategy-docs.patch

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

Looks good, thanks for doing the copying.

::: js/src/doc/JITOptimizations/Outcomes.md
@@ +234,5 @@
> +more than one level.  The first level of recursion is inlineable.
> +
> +### CantInlineHeavyweight
> +
> +Unable to inline function call. The interpreted callee function contains

I had categorized the outcomes of inlining into 'unable' and 'unwilling', and the strategy page still refers to this distinction. Looks like you changed everything to 'unable', could you restore the 'unwilling' wording?
Attachment #8641177 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #21)
> Comment on attachment 8641177 [details] [diff] [review]
> I had categorized the outcomes of inlining into 'unable' and 'unwilling',
> and the strategy page still refers to this distinction. Looks like you
> changed everything to 'unable', could you restore the 'unwilling' wording?

I just checked the wiki.  The wording there uses 'unable' for everything.  Can you refer me to the unwilling wording?
Trivial rebase.  Forwarding r+.
Attachment #8641177 - Attachment is obsolete: true
Attachment #8643147 - Flags: review+
Assignee: nobody → kvijayan
Whiteboard: [leave open]
Keywords: checkin-needed
Patch fixing the markdown for the jitopt strategy docs, and adding a few more jitopt outcome docs.

Wiki pages have been updated with markdown generated output for both of these.
Attachment #8654366 - Flags: review?(shu)
Comment on attachment 8654366 [details] [diff] [review]
update-jit-optimization-docs.patch

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

::: js/src/doc/JITOptimizations/Strategies.md
@@ -77,5 @@
>  after it was first assigned, is likely a constant property.  It then directly
>  inlines the value of the property into hot code that accesses it. For
>  example, in the following code:
>  
> -```language-js

Why the removal of ```? Was it not being rendered properly?
Attachment #8654366 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #27)
> Why the removal of ```? Was it not being rendered properly?

Yeah, markdown proper didn't handle it, at least the one I pulled down with apt on my debian box.  Went for a more traditional syntax.
More jit coach outcome documentation.  This completes nominal documentation for all the outcomes in the list.
Attachment #8658329 - Flags: review?(shu)
Comment on attachment 8658329 [details] [diff] [review]
jit-coach-optimization-docs.patch

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

Thanks!

::: js/src/doc/JITOptimizations/Outcomes.md
@@ +286,4 @@
>  ### ICGetPropStub_UnboxedRead
> +
> +An inline cache property read which retrieves an unboxed
> +value from an object.

I think this should be "retrieves a value from an unboxed object".

@@ +307,5 @@
> +
> +An inline cache property read which retrieves a
> +shadowed property from a DOM object.  A shadowed
> +property is an inbuilt DOM property that has been
> +overwritten by JS code.

nit: What's your character width set to? These seem real narrow.
Attachment #8658329 - Flags: review?(shu) → review+
(In reply to Shu-yu Guo [:shu] from comment #32)
> Comment on attachment 8658329 [details] [diff] [review]
> jit-coach-optimization-docs.patch
> nit: What's your character width set to? These seem real narrow.

100 chars.  I don't know why, I just ended up writing those with a narrow wordwrap.  Fixed.
Closing for now.  We can open a new bug if we need to add more docs.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.