Jan de Mooij [:jandem] Reporter
	Description • 7 years ago

Bug 1344463 will improve the six-speed object-literal-ext-es6 test a lot. To optimize it more we should do the following:

* Do something similar for JSOP_INITPROP's slow path as what I did for JSOP_INITELEM (use the SetProp IC). Or (even faster) figure out how we can make IonBuilder's fast path for JSOP_INITPROP handle this case.

* Optimize JSOP_TOID. I wonder if we could turn it into JSOP_TOPRIMITIVE, then we can easily nop it in IonBuilder for strings and symbols.

	Tom S [:evilpie] Assignee
	Comment 2 • 7 years ago

We could change this to JSOP_TOPRIMITIVE, but I feel like the current name with just allowing strings/symbols is a bit less surprising.

	Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)
	Comment 3 • 7 years ago

Helping :evilpie send this to Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70a5a2566a6ab1658fd7e3bc815072ae2df26a81

	Tom S [:evilpie] Assignee
	Comment 4 • 7 years ago

There is a 32-bit only failure on jit-test/tests/wasm/conversion.js that I don't understand yet. https://treeherder.mozilla.org/#/jobs?repo=try&author=evilpies@gmail.com&selectedJob=110563876

	Tom S [:evilpie] Assignee
	Comment 5 • 7 years ago

I can reproduce this now, even wiht just the first patch. We definitely seem to create more ICs now. I think this is somehow caused by this: http://searchfox.org/mozilla-central/source/js/src/jit-test/lib/wasm.js#114, but I am not sure.

	Tom S [:evilpie] Assignee
	Comment 6 • 7 years ago

Comment on attachment 8881864 [details] [diff] [review]
[WIP] Nop jsop_toid for string/symbol

r+ over IRC

	Tom S [:evilpie] Assignee
	Comment 7 • 7 years ago

Turns out just the TOID patch doesn't apply cleanly. We can now fold SetElem with a constant string (because there is no more ToId instruction) to SetProp, which didn't behave correctly in the fallback case.

	Tom S [:evilpie] Assignee
	Comment 8 • 7 years ago

We should probably change the HandleId parameter to PropertyName in InitPropertyOperation.

	Jan de Mooij [:jandem] Reporter
	Comment 9 • 7 years ago

Comment on attachment 8886627 [details] [diff] [review]
v1 - Nop jsop_toid for string/symbol

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

r=me but I think the suggested fix below is more in line with the rest of the code.

::: js/src/jit/IonIC.cpp
@@ +223,5 @@
>      }
>  
>      jsbytecode* pc = ic->pc();
> +
> +    if (*pc == JSOP_INITELEM) {

I think JSOP_INITHIDDENELEM can also show up here?

@@ -238,5 @@
> -        if (*pc == JSOP_INITGLEXICAL) {
> -            RootedScript script(cx, ic->script());
> -            MOZ_ASSERT(!script->hasNonSyntacticScope());
> -            InitGlobalLexicalOperation(cx, &cx->global()->lexicalEnvironment(), script, pc, rhs);
> -        } else {

What if we just add:

  } else if (IsPropertyInitOp(JSOp(*pc))) {

With a call to InitPropertyOperation? InitPropertyOperation takes the JSOp, but if we add the INITELEM ops to GetInitDataPropAttrs I think everything should work. That matches what we do for SETPROP, we can keep the code structure the same as before, and it should be a bit more efficient than going through InitElemOperation when we already have a PropertyName.

	Pulsebot
	Comment 10 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea49e7dc13d5
Nop jsop_toid for string/symbol. r=jandem

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 11 • 7 years ago

Backed out for frequently crashing with js::GetInitDataPropAttrs(JSOp) in devtools tests on Windows, e.g. browser_rules_edit-selector_04.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/afbb815ae8bc34f94a4a08e72e59a08f0f00a46e

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ea49e7dc13d5eab3a7ce1e5cb3aef86267fa910c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=114622517&repo=mozilla-inbound

11:53:27     INFO - Selecting the ruleview sidebar
11:53:27     INFO - Selecting the node for 'p'
11:53:27     INFO - TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-selector_04.js | No selectorhighlighter exist in the rule-view - 
11:53:27     INFO - Test creating selector highlighter
11:53:27     INFO - Clicking on a selector icon
11:53:27     INFO - TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-selector_04.js | The selectorhighlighter instance was created - 
11:53:27     INFO - TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-selector_04.js | The toggle event says the highlighter is visible - 
11:53:27     INFO - Test editing existing selector fields
11:53:27     INFO - Focusing an existing selector name in the rule-view
11:53:27     INFO - Waiting for event: 'focus' on [object HTMLDivElement].
11:53:27     INFO - Clicking on editable field to turn to edit mode
11:53:27     INFO - Got event: 'focus' on [object HTMLDivElement].
11:53:27     INFO - Editable field gained focus, returning the input field now
11:53:27     INFO - TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-selector_04.js | The selector editor got focused - 
11:53:27     INFO - Waiting for rule view to update
11:53:27     INFO - Entering a new selector name and committing
11:53:27     INFO - Buffered messages finished
11:53:27    ERROR - TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_edit-selector_04.js | application terminated with exit code 1
11:53:27     INFO - runtests.py | Application ran for: 0:02:39.987000
11:53:27     INFO - zombiecheck | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpthsayzpidlog
11:53:27     INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/DGGdfVMYRuCfEfbuRzn9-Q/artifacts/public/build/firefox-56.0a1.en-US.win32.crashreporter-symbols.zip
11:53:32     INFO - mozcrash Copy/paste: c:\slave\test\build\win32-minidump_stackwalk.exe c:\users\cltbld\appdata\local\temp\tmpn7k14g.mozrunner\minidumps\4ba3c116-a2a1-425d-bd0d-6ad7c07001af.dmp c:\users\cltbld\appdata\local\temp\tmpg804oo
11:53:40     INFO - mozcrash Saved minidump as c:\slave\test\build\blobber_upload_dir\4ba3c116-a2a1-425d-bd0d-6ad7c07001af.dmp
11:53:40     INFO - mozcrash Saved app info as c:\slave\test\build\blobber_upload_dir\4ba3c116-a2a1-425d-bd0d-6ad7c07001af.extra
11:53:40     INFO - PROCESS-CRASH | devtools/client/inspector/rules/test/browser_rules_edit-selector_04.js | application crashed [@ js::GetInitDataPropAttrs(JSOp)]
11:53:40     INFO - Crash dump filename: c:\users\cltbld\appdata\local\temp\tmpn7k14g.mozrunner\minidumps\4ba3c116-a2a1-425d-bd0d-6ad7c07001af.dmp
11:53:40     INFO - Operating system: Windows NT
11:53:40     INFO -                   6.1.7601 Service Pack 1
11:53:40     INFO - CPU: x86
11:53:40     INFO -      GenuineIntel family 6 model 62 stepping 4
11:53:40     INFO -      8 CPUs
11:53:40     INFO - 
11:53:40     INFO - GPU: UNKNOWN
11:53:40     INFO - 
11:53:40     INFO - Crash reason:  EXCEPTION_BREAKPOINT
11:53:40     INFO - Crash address: 0x6028105c
11:53:40     INFO - Process uptime: 160 seconds
11:53:40     INFO - 
11:53:40     INFO - Thread 0 (crashed)
11:53:40     INFO -  0  xul.dll!js::GetInitDataPropAttrs(JSOp) [Interpreter.cpp:ea49e7dc13d5 : 4774 + 0xb]
11:53:40     INFO -     eip = 0x6028105c   esp = 0x0029ca74   ebp = 0x0029cc4c   ebx = 0x322bf478
11:53:40     INFO -     esi = 0x0029cc88   edi = 0x322bf478   eax = 0x6de317e8   ecx = 0x0000005f
11:53:40     INFO -     edx = 0x0029d000   efl = 0x00000287
11:53:40     INFO -     Found by: given as instruction pointer in context
11:53:40     INFO -  1  0x174d5626
11:53:40     INFO -     eip = 0x174d5626   esp = 0x0029cc54   ebp = 0x00000001
11:53:40     INFO -     Found by: previous frame's frame pointer
11:53:40     INFO -  2  xul.dll!intrinsic_IsPossiblyWrappedTypedArray [SelfHosting.cpp:ea49e7dc13d5 : 1009 + 0x13]
11:53:40     INFO -     eip = 0x60411438   esp = 0x0029ccd8   ebp = 0x0029ccec
11:53:40     INFO -     Found by: stack scanning
11:53:40     INFO -  3  0x18ad07d7
11:53:40     INFO -     eip = 0x18ad07d7   esp = 0x0029ccf4   ebp = 0x00000000
11:53:40     INFO -     Found by: previous frame's frame pointer

	Tom S [:evilpie] Assignee
	Comment 12 • 7 years ago

I guess there are just more kinds of http://searchfox.org/mozilla-central/search?q=JOF_PROPINIT opcodes that can appear here: probably INITELEM_INC or INITELEM_INC or even INITALIASLEXICAL.

	Pulsebot
	Comment 13 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d2ba61c697a
Nop jsop_toid for string/symbol. r=jandem

	Tom S [:evilpie] Assignee
	Comment 14 • 7 years ago

JIT_OPTION_forceInlineCaches=true showed various failures related to INITELEM_INC, so I just made us call InitArrayElemOperation.

	Jan de Mooij [:jandem] Reporter
	Comment 15 • 7 years ago

Bug 1381438 should fix your 32-bit jit-test failure. Good catch.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1d2ba61c697a

	Jan de Mooij [:jandem] Reporter
	Comment 18 • 7 years ago

Comment on attachment 8887084 [details] [diff] [review]
v1 - Use an IC for JSOP_INITPROP in Ion

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

Great to have this.

::: js/src/jit/IonBuilder.cpp
@@ +6460,5 @@
> +        // This is definitely initializing an 'own' property of the object, treat
> +        // it as an assignment.
> +        MOZ_TRY(jsop_setprop(name));
> +    } else {
> +        if (*pc != JSOP_INITPROP) {

Nit: Baseline uses the same IC code for the HIDDEN and LOCKED ops, so I assume the CacheIR code for this does the right thing? r=me either way.

	Pulsebot
	Comment 19 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aed7f559822
Use an IC for JSOP_INITPROP in Ion. r=jandem

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9aed7f559822

	Tom S [:evilpie] Assignee
	Comment 21 • 7 years ago

We can't actually test this at the moment, because INITHIDDENPROP requires async or class constructor support. So I am going to add that as well in another bug.

	Tom S [:evilpie] Assignee
	Comment 22 • 7 years ago

Actually we can already trigger this code by using a custom constructor in a class. Because the things that we end up calling initprop on is only going to increase, I changed the code from using useSlowPath to useFastPath.

	Jan de Mooij [:jandem] Reporter
	Comment 23 • 7 years ago

Comment on attachment 8888256 [details] [diff] [review]
v2 - Remove MInitProp

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

Coolio.

	Pulsebot
	Comment 24 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4f222d9501
Remove MInitProp from Ion. r=jandem

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 25 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7e4f222d9501