Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[JS] Std.string optimization changed behavior #2716

Closed
back2dos opened this Issue Mar 5, 2014 · 20 comments

Comments

Projects
None yet
3 participants
Member

back2dos commented Mar 5, 2014

Std.string is just dropped for strings since this change. As a result Std.string(null) == null, which makes a big difference to the previous Std.string(null) == "null". I suggest using stringv for the String case.

Owner

Simn commented Mar 5, 2014

Ouch, this is actually specified. It is not caught by the unit tests because they run in -no-opt mode by default.

Owner

ncannasse commented Mar 5, 2014

Todays teaching : We should have Travis run the unit tests without -no-opt as well.

Owner

Simn commented Mar 5, 2014

Actually we don't test it anyway. We have Std.string(null), but that's not checking the String optimization case. We should add Std.string((null : String)) for that.

Owner

ncannasse commented Mar 5, 2014

var x : String = null; Std.string(x) should make sure that nothing is optimized too much (until we have constant propagation at least)

Owner

Simn commented Mar 5, 2014

That's the same test as mine.

So should we revert the String optimization then?

Member

back2dos commented Mar 5, 2014

Well, I think ""+value is a good short hand.

As for the question how to make the spec, I think
Std.string(['foo'][Std.random(1)+1]) should survive any optimization for
the foreseeable future :D

On Wed, Mar 5, 2014 at 11:21 PM, Simon Krajewski
notifications@github.comwrote:

That's the same test as mine.

So should we revert the String optimization then?

Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2716#issuecomment-36801865
.

Owner

Simn commented Mar 5, 2014

Ah, I didn't see we already had that code in place for the other types. I'll change it accordingly.

Owner

Simn commented Mar 5, 2014

No good: null should be undefined (that's reverse).

Member

back2dos commented Mar 6, 2014

Right. JavaScript. Yay.

Owner

ncannasse commented Mar 6, 2014

['foo',null] should work ;)

Owner

Simn commented Mar 6, 2014

But is that a bug in the test or in the optimization? I'm never quite sure what our rules regarding JS undefined are.

Member

back2dos commented Mar 6, 2014

The main issue is that the js.Boot implementation will return "null" for undefined while the optimized version makes it "undefined" (not just for Strings but for all js primitives).

I'm pretty sure I have some code along the lines of someMap[Std.string(someNullableThing)] (based on the assumption that all null keys are equal) running in production and am not looking forward to finding out just where that is ...

If the optimization is about speed then it might make sense to have a js.Boot.__string_primitive that deals with the null / undefined case and returns ""+arg otherwise. Shaving off the indirection + type inspection seems to make the difference. You can see the numbers at http://try.haxe.org/#27BCA - these are the ones I got:

------- START --------- (Chrome)
using [foo]
"direct add => foo" took 0.018000125885009766
"fastInline => foo" took 0.01399993896484375
"fast => foo" took 0.01399993896484375
"fast_js.Boot.__string_primitive => foo" took 0.11300015449523926
"Std.string => foo" took 0.38499999046325684
"Std.string Dynamic => foo" took 0.3840000629425049
using []
"direct add => undefined" took 1       ////// This is wrong ...
"fastInline => null" took 0.6659998893737793
"fast => null" took 0.6059999465942383
"fast_js.Boot.__string_primitive => null" took 0.6440000534057617
"Std.string => null" took 0.744999885559082
"Std.string Dynamic => null" took 0.629000186920166 

------- START --------- (Firefox)
using [foo]
"direct add => foo" took 0.011999845504760742
"fastInline => foo" took 0.012000083923339844
"fast => foo" took 0.010999917984008789
"fast_js.Boot.__string_primitive => foo" took 0.012000083923339844
"Std.string => foo" took 0.8819999694824219
"Std.string Dynamic => foo" took 0.880000114440918
using []
"direct add => undefined" took 0.28999996185302734      ////// ... but at least consistent
"fastInline => null" took 0.03600001335144043
"fast => null" took 0.03600001335144043      //////  !!!! SUCH AMAZING !!!!
"fast_js.Boot.__string_primitive => null" took 0.04399991035461426
"Std.string => null" took 0.6549999713897705
"Std.string Dynamic => null" took 0.6460001468658447

It seems that the optimization is either not enabled or not yet available on try.haxe.org. What's surprising is that on both browsers the null-check actually provides better results than the plain empty string addition. Reminds me of the good old days of tricking the AVM2 into going an order of magnitude faster with trivial changes.
The path access (fast_js.Boot.__string_primitive) seems to tax Chrome quite a bit, but js-flatten should help remedy this. Otherwise inlining is an option as the numbers suggest.

@Simn Simn closed this in a2fcda3 Mar 6, 2014

Owner

ncannasse commented Mar 6, 2014

There's indeed things to fix for null/undefined, but I think it's worth it.
I think the fastInline is good, and we can actually skip the null check in Flash if the value is not null.

@ncannasse ncannasse reopened this Mar 6, 2014

Owner

ncannasse commented Mar 6, 2014

I would this to be fixed for 3.1.1, this is a quite important optimization

@Simn Simn was assigned by ncannasse Mar 6, 2014

Owner

Simn commented Mar 6, 2014

Why? It wasn't there before, so it's fine to omit it for now. Could we please focus on stability for this bugfix release instead of trying to squeeze in optimizations?

Owner

ncannasse commented Mar 6, 2014

This really improves the speed for important things such as serialization. Now that we have a good understanding of the problem, it's fine. I can take on the issue if you wish.

@Simn Simn assigned ncannasse and unassigned Simn Mar 6, 2014

Owner

Simn commented Mar 6, 2014

Sure, I'll focus on writing mean unit tests for it then. ;)

@Simn Simn added this to the 3.1.1 milestone Mar 6, 2014

Owner

Simn commented Mar 9, 2014

Are you going to look into this soon or do you not consider it blocking for 3.1.1?

Owner

ncannasse commented Mar 9, 2014

I'll do it soon, when is 3.1.1 planned ?

Owner

Simn commented Mar 9, 2014

I wanted to do it today, but looking at the UInt issue we have to delay a bit anyway until we know what we want to do there. Still I would like to get it done within the next three days if possible.

@ncannasse ncannasse closed this in 3788bd9 Mar 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment