BaselineCompiler: Add optimized stub for JSOP_THIS null/undef => "global this" case

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: djvj, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
when JSOP_THIS encounters a null or undefined, it computes the |this| object from the currenet global.  The computed |this| object should be stable for that global.

This means that when the incoming this is null or undefined, the resulting this object can be cached and returned via an optimized stub.
(Reporter)

Comment 1

6 years ago
Created attachment 709350 [details] [diff] [review]
Patch
Attachment #709350 - Flags: review?(jdemooij)
Is the difference measurable? JM (at least without TI) stubs JSOP_THIS, it would be nice if we could do the same..
(In reply to Jan de Mooij [:jandem] from comment #2)
> Is the difference measurable? JM (at least without TI) stubs JSOP_THIS, it
> would be nice if we could do the same..

To be clear, it calls a stub if the value is a primitive.
(Reporter)

Comment 4

6 years ago
When I run with just baseline (disabling Ion), it seems to have about a 0.1/16 ms (or 1/160) difference in 3d-cube, but it's noisy so I can't say for sure.

I saw it hitting about 5000 thousand times in 3d-cube before Ion got entered.  The implementation was simple and would take about a half hour or so, so I went ahead and did it.  To be honest I was expecting a bigger impact but apparently not.

It might help 3d-cube a bit to have Ion do the same thing.  It handles a particular call to |CreateP| in 3d-cube which is called in non-constructing form, but CreateP uses this.  The presence of |this| in CreateP without a definitely-typed object for thisval makes ion not compile CreateP, which means the engine ends up doing an Ion=>JM invoke for all the calls to CreateP from within Ion.  Eliminating this and allowing CreateP to be inlined into the caller would probably be a definite win.

However, given that we don't expect it to be too common for callers to call functions in non-constructing form if the function uses |this|, it's probably not a huge priority.
Comment on attachment 709350 [details] [diff] [review]
Patch

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

I think we should replace the IC with a direct VM call if |this| is not an object. The slowest part of the IC is probably recovering the script + pc, if we use a VM call we can just pass these to the stub. (The arguments checks should give us type information about |this|)
Attachment #709350 - Flags: review?(jdemooij)
(Reporter)

Comment 6

6 years ago
Well, the fallback stub doesn't strictly need to get the script or pc to call BoxNonStrictThis.  If we're not doing any other optimizations, we can just keep the fallback stub as is and remove the script/pc retrieval from it.
(Reporter)

Comment 7

6 years ago
Created attachment 709758 [details] [diff] [review]
Remove unnecessary script/pc access from JSOP_THIS fallback stub.
Attachment #709350 - Attachment is obsolete: true
Attachment #709758 - Flags: review?(jdemooij)
Comment on attachment 709758 [details] [diff] [review]
Remove unnecessary script/pc access from JSOP_THIS fallback stub.

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

::: js/src/ion/BaselineIC.cpp
@@ +1066,5 @@
>  
>  static bool
>  DoThisFallback(JSContext *cx, ICThis_Fallback *stub, HandleValue thisv, MutableHandleValue ret)
>  {
> +    FallbackICSpew(cx, stub, "NewArray");

Nit: "This"
Attachment #709758 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 9

6 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/4b013896bfd5
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.