841859 - BaselineCompiler: Add optimized stub for String < or > String

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

string-validate-input hits these 24k times in total.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Patch

Patch inlines comparison of linear strings which are <64 code units in length.  The vast majority of comparisons should fall into this class and the inline path should fall under the vmcall overhead for those cases.

To avoid register pressure on x86, the comparison loop stores the lengths of both strings in one register, and uses some bit tricks (described in comments) to do the comparison.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 715343 [details] [diff] [review]
Patch

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

I'm not sure it's worth it. I see a <= 0.2 ms win on validate-input on x86 with --no-jm with/without parallel compilation, no other benchmarks we know need this, and the other JITs also don't specialize this case. What do you think?

Comment 3

[Kannan Vijayan [:djvj]]

I think having a stub is worth it, if only to convey the (String, String) specialization for the compare.  Once we get rid of stack typesets this info will have to come from here.

Maybe the fancy assembly in the stub may not be ;)

OTOH, I do think the fancy assembly would be interesting to use in Ion.

Comment 4

[Brian Hackett [Laid off!]]

Ion should be able to determine the two operands are strings, even without StackTypeSets.  There shouldn't be any loss in functionality with bug 804676, it will just be restructuring things so that the types we now explicitly track and keep around in StackTypeSets are instead encoded in the MIR.

Comment 5

[Kannan Vijayan [:djvj]]

In that case we can probably leave this be for now.  I'm not convinced we shouldn't have a stub, though.  If for whatever reason we have string-heavy code that doesn't spend a lot of time in Ion, we'll end up taking a _really_ slow path for it every time - with stack iter, a table lookup, stub-attachment checks, the whole lot.

We should try to avoid that just on principle.

Comment 6

[Kannan Vijayan [:djvj]]

Comment on attachment 715343 [details] [diff] [review]
Patch

Forgot to cancel this review a long time ago..

Comment 7

[Mayank Bansal]

Steve, still worth doing?

Comment 8

[Iain Ireland [:iain]]

Anba did some of this work in bug 1538692, so we do attach and transpile stubs in this case. Right now, the only case we handle in jitcode (for relational comparison) is comparing two strings pointing to the same instance. In all other cases, we do a VM call. (There's also an Ion-only optimization to compare strings against single chars.)

We could hypothetically expand MacroAssembler::compareStrings to handle eg short linear strings. I don't remember seeing jit::StringsCompare a lot in profiles, though, so it's not clear that it's especially important.