Closed Bug 837359 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: djvj, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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.
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)
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.
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+
https://hg.mozilla.org/projects/ionmonkey/rev/4b013896bfd5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: