1360369 - Unify implementation of JSOP_CHECKOBJCOERCIBLE and the self-hosted RequireObjectCoercible function

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Shu-yu Guo [:shu]]

We have both a specialized JSOP and a self-hosted function for checking if an object is coercible. anba over in bug 1360220 removed uses of calling out to the self-hosted function in generated bytecode. I'd like to only have one implementation. We should remove the self-hosted function and special case calls to |RequireObjectCoercible| (renamed to something else, most likely) when emitting bytecode for self-hosted functions, like |allowContentIter|, to emit JSOP_CHECKOBJCOERCIBLE instead.

Comment 1

[Tom S [:evilpie]]

We should do this. We have a call to RequireObjectCoercible in all string methods. With this fixed we should be able to be in a place where for many string methods we aren't calling another functions from Baseline and Warp.

Comment 2

[Tom S [:evilpie]]

I prototyped this, but sadly it makes an already bad error message worse.
Before we get "can't convert null to object", when using CheckObjCoercible: "this is null". For reference Chrome gives "String.prototype.blink called on null or undefined", which is much better.

Comment 3

[Tom S [:evilpie]]

Attachment: Bug 1360369 - Remove RequireObjectCoercible from self-hosted String methods and improve error messages. r?jandem

Depends on D79615

Comment 4

[Tom S [:evilpie]]

Attachment: Bug 1360369 - Improve error message for C++ implemented String function and add a test. r?jandem

Depends on D79655

Comment 5

[Tom S [:evilpie]]

I mostly wrote these patches for the improved error messages. If we actually want to improve the performance with Warp/baseline it might make sense to add a IsNullOrUndefined JSOp to replace the two strict equality comparisons. Luckily we already have this as a MIR/LIR instruction.

Comment 6

[Tom S [:evilpie]]

I hadn't noticed this before locally. But jit-test/tests/ion/dce-with-rinstructions.js fails with Assertion failure: input()->isRecoveredOnBailout() == mustBeRecovered_. The failing test seems to be rstr_split.

Comment 7

[Tom S [:evilpie]]

Turns out changing the call to and if + call would get us right over the inlinig limit for String_split. Setting --ion-limit-script-size=off when running the test is a simple fix suggested by Jan.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a634c5346591
Remove RequireObjectCoercible from self-hosted String methods and improve error messages. r=jandem
https://hg.mozilla.org/integration/autoland/rev/7b278c9988df
Improve error message for C++ implemented String function and add a test. r=jandem

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a634c5346591
https://hg.mozilla.org/mozilla-central/rev/7b278c9988df