583939 - Optimize safe_add() as 32-bit addition [JSOP_SUNSPIDER]

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Roy Frostig :froystig]

There is a JS function called safe_add() in crypto-sha1 and crypto-md5 that custom-implements 32-bit addition in terms of 16-bit JS operations "to work around bugs in some JS interpreters."

I conjecture that ours is not one such buggy interpreter.  Hence, Andreas suggests that it would be nice to monitor for such script workarounds and lower them back into regular old 32-bit addition.  We can catch these when trace-recording function calls, say, by noticing functions that happen to be bytecode-identical to safe_add().

Comment 1

[David Mandelin [:dmandelin]]

Is there a risk that we can be accused of benchmark-gaming if we have code that looks for and specially optimizes a specific function in SunSpider? Recognizing a common function in web code seems fair, but if we do a function that is only known to occur in SunSpider I am a little concerned.

Comment 2

[Brendan Eich [:brendan]]

Word on the street is some competitors (must I mean closed source competitors? I could be missing open-source tuning for SunSpider) are doing similar and worse hacks. Anyone who remembers the SPEC wars of the '90s knows the game theory.

/be

Comment 3

[Andreas Gal :gal]

safe_add results in over 52,000 hits on google. The code is used all over the web.

Comment 4

[Brendan Eich [:brendan]]

http://www.google.com/codesearch?q=lang%3Ajs+safe_add&hl=en&btnG=Search+Code hits 716 matches.

/be

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> safe_add results in over 52,000 hits on google. The code is used all over the
> web.

Sounds good then.

Comment 6

[Nicholas Nethercote [inactive]]

This would probably render bug 575529 moot.  This isn't necessarily a bad thing.

Comment 7

[Brian Hackett [Laid off!]]

FWIW neither JSC nor V8 special cases safe_add.  If I mutate the code so it does some other rubbish thing (with the same number of ALU ops), the time for JSC/V8 is unchanged.  I am also concerned about the issues raised in comment 1; regardless of how often safe_add is used on the web, it still seems like benchmark-gaming.  If you can tweak a few bits in a benchmark and introduce a major performance regression on some engines but not others, it makes those engines look bad (see bug 580018).

Comment 8

[Brian Hackett [Laid off!]]

Er, bug bug 580018, comment 10.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: patch specializing safe_add() in ExprFilter (against TM 52847:857ab0bfeb22)

FWIW, here's a patch that specializes safe_add() in the ExprFilter.  This is arguably less bogus than specializing on the bytecode, but not much.  It also helps us a lot less than specializing on the bytecode because with this patch heaps of code gets generated that ends up dead, whereas with a bytecode approach that code wouldn't be generated at all.  And compile-time dominates crypto-md5 for us.

But it probably shouldn't be applied.

Comment 10

[Nicholas Nethercote [inactive]]

Time to WONTFIX this wee beastie.