561080 - Intern less aggressively on property lookup

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Virtual Machine, enhancement)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

Property lookups (indexed and non-indexed) require that the key used for the query be interned.  As far as I can tell, if the key was not already interned, then there should be no value associated with the key, which means that we are going to produce the undefinedAtom and could avoid doing the intern in the first place.

I suggest adding a family of methods allowing one to inspect whether a key already is present in the intern table: one that consumes a char*, and another that consumes a uint_t.

One risk here is that I may be wrong in my hypothesis that if a key was not already interned, then there should be no value associated with the key.  (If you know of a counter-example to this hypothesis, please comment!)

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Note that there is already a AvmCore::findInternedString method that is only included if DEBUGGER if #defined; perhaps we could turn that on permanently and then I could leverage that to get the desired effect?

Comment 2

[Edwin Smith]

counter example:

obj = {}
obj["foobar"] = 1    // this "foobar" is interned
print(obj["foo" + "bar"]) // freshly minted string "foobar" won't be marked interned yet

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #2)
> counter example:
> 
> obj = {}
> obj["foobar"] = 1    // this "foobar" is interned
> print(obj["foo" + "bar"]) // freshly minted string "foobar" won't be marked
> interned yet

Ah, I was not clear in my description.  I should have said "if there is not already an interned string equal to the key, then there should be no value associated with the key."

In your example above, the value of ("foo" + "bar") is indeed not itself interned, but there is a "foobar" entry in the intern table, so it would pass my test and lookup would proceed.

The case I'm trying to catch is something like print(obj["baz" + "foo"]), where now the string "bazfoo" is presumably not in the intern table, and therefore I am hypothesizing that we can bottom out early, returning the undefinedAtom, without having to traverse obj's property table.

Comment 4

[Steven Johnson]

This sounds like a promising inquiry -- I don't see any obvious flaws, but this will of course require some careful evaluation to be certain... one caution: instead of "one that consumes a char*" you should think "one that consumes a unicode string", which (given our string class) probably needs to be two functions, one for 8-bit Latin1 and one for UTF16.

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to comment #4)
> This sounds like a promising inquiry -- I don't see any obvious flaws, but this
> will of course require some careful evaluation to be certain... one caution:
> instead of "one that consumes a char*" you should think "one that consumes a
> unicode string", which (given our string class) probably needs to be two
> functions, one for 8-bit Latin1 and one for UTF16.

Yes, the family of methods is likely to need to be richer than what I wrote initially.  Just searching for "AvmCore::internString" in AvmCore.cpp gives me an idea of how many variants we might want to support.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.