JS generator handles obj[""] badly

[back2dos]

There seems to be something in the JS generator that allows for object["constant"] to become object.constant. Unfortunately object[""] becomes object. thus causing a syntax error. Example code:

class Main {
    static function main() ['' => ''];
}

In general, adding ['' => ''].keys().next() == ''; to Map.unit.hx might not be a bad idea.

[Simn]

That probably means that valid_js_ident returns true for empty strings.

[Simn]

What do you know, that test actually fails on flash, java and C#.

[Simn]

@smerkyg: This could be a StringMap regression.

[back2dos]

Oh, wow. There's nothing like a useless edge case.

[Simn]

On Flash this is a regression. The problem is that next() only returns a value if hasNext() has been called before. This is also why the unit tests don't catch this problem (which is unrelated to empty strings) because we only test keys() within a for loop.

We had this issue on python too in #3191. Unfortunately the regression test for that only covered IntMap and iterator and thus missed the problem in the new StringMap implementation.

[SmerkyG]

So, the constructor for StringMapKeysIterator should probably call hasNext(), instead of setting nextIndex to zero to start.

[Simn]

That's the solution I thought of too. I'll change it accordingly.

[Simn]

What about isReserved? It is set to false in the constructor and true in hasNext if that condition holds. Which should it be after construction?

[SmerkyG]

Yeah that also has to be updated. It keeps track of whether you're iterating the 'normal' keys or have moved on to the 'reserved' keys. Don't forget to apply the same patch to StringMapValuesIterator btw. I have to apply this to my proposed IntMap iterator pull rq too.

[SmerkyG]

I mean that isReserved = false; should be set, and then you should call hasNext

[waneck]

I wish we had adopted .NET-like Enumerators instead of Iterators - the interface ( { moveNext:Void->Bool, current:T } ) is much more clear of what does what than hasNext()/next()

[SmerkyG]

I agree, but sadly that's a relic from AS3.

[Simn]

I have fixed StringMap, but ObjectMap has the same issue. Trying to fix it the same way lead to errors all over the place.

[SmerkyG]

Yeah unfortunately ObjectMap is written poorly. It needs an overhaul to use an iterator similar to the new (fixed) version on StringMap

[SmerkyG]

Basically the problem is that it doesn't store nextIndex.

[SmerkyG]

Before we get so deep into this, are you really ALLOWED by the specification to call next() before you call hasNext()?

[Simn]

Its hasNext actually changes state!

[Simn]

With names like that you are clearly allowed to call next() before hasNext().

[ncannasse]

We could only specify that the following works as intended:

while( it.hasNext() ) { var v = it.next(); }

[Simn]

I would find that really awkward, especially if Flash is the only target which has that problem.

[Simn]

I'm in a bind here. I really hate target-inconsistencies like this and would definitely like to address it, but on the other hand I can already see flash users with foam in their mouths because they might lose a precious nanosecond in their per-frame ObjectMap iteration.

We dealt with the original issue and the follow-up regression on flash's StringMap. The rest is not a regression and while I really don't like it (repeating myself) I've decided not to address it for 3.2.

Expect this one of the first things I'll look into for 3.3 so we have enough time to send in the nanosecond-rescue-squad.

[back2dos]

I do see your point, but I think as far as the cross platform standard library goes, consistency trumps performance. The more so, if sacrificing a few nanoseconds is an issue, while other obvious and significant options are far from being exhausted.

Storing the reserved object in an instance pointer gives you a factor 2-3 speedup in lookup: http://try.haxe.org/#3aFe3

Similarly, adding an inline to ObjectMap's iterator gives you almost factor 2 speedup: http://try.haxe.org/#1162e

Then there's also this article recently posted in haxelang, where maintaining a key list will give you iteration speed up (at the expense of overhead for read and write operations I would presume).

There are many ways to tackle performance issues, but you get a much bigger bang when you know your specific use cases. It's not the std libs job to try to anticipate all of them, but instead to implement a consistent behavior with the least overhead possible. And then still, I think this is the kind of stuff that people who actually care about that kind of performance can fix with pull request accompanied with the numbers to back their optimization.

As a side note: what leaves me somewhat puzzled in this situation is why I have been arguing for over a year to have pattern matching on getters, when nobody seems to see the slightest issue in implementing hasNext without referential transparency :P Seriously though, I think that is really a great trip wire. We're just lucky working on iterators directly is less popular in Haxe than other languages.

As for the issue at hand: I'm not claiming that this particular inconsistency requires a fix ASAP. In fact I understand if its delayed because fixing it probably rather tedious. But performance is simply not a valid argument here.

On Thu, Feb 26, 2015 at 8:47 AM, Simon Krajewski notifications@github.com wrote:

I'm in a bind here. I really hate target-inconsistencies like this and would definitely like to address it, but on the other hand I can already see flash users with foam in their mouths because they might lose a precious nanosecond in their per-frame ObjectMap iteration.

We dealt with the original issue and the follow-up regression on flash's StringMap. The rest is not a regression and while I really don't like it (repeating myself) I've decided not to address it for 3.2.

Expect this one of the first things I'll look into for 3.3 so we have enough time to send in the nanosecond-rescue-squad.

— Reply to this email directly or view it on GitHub https://github.com/HaxeFoundation/haxe/issues/3908#issuecomment-76135467 .

[SmerkyG]

@back2dos moving the rh to an instance variable was not what improved your performance. (I tested this when writing the new StringMap and we decided that it does not help much) TryHaxe is using a non-development branch and therefore you are comparing the 'old' StringMap to my and Nicolas's new one that will ship with 3.2, which is, as you have seen, several times faster. Also, adding inline to ObjectMap's iterator is not what improved your performance. It was changing the return type to NativePropertyIterator from Iterator. This is exactly the change I've been lobbying for across all the map types for a while now, and is referenced in #3366 and #3147.

@Simn in the future you may consider that even though it slows things down a bit to 'fix' the hasNext on ObjectMap, if you can eventually return an ObjectMapKeysIterator and ObjectMapValuesIterator instead of generic Iterators, it will make iteration run overall way faster than in 3.1.

[Simn]

Moving towards Haxe 4, we will no longer look into fixing old Flash/SWF issues. We are still going to keep the generator up to date with all new Haxe features, but we don't have the resources to work on flash-specific bugs. Pull requests to fix them are still going to be reviewed and possibly accepted.