596490 - TM: create "normalized" jsid type for safety

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement, P5)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Nick brought this up while we were working on making getelem fast in the tracer (bug 593931).

Some parts of the engine effectively require that "normalized" jsids are given to them. To codify this in a safe way, we should create an opaque type, distinct from jsid, that indicates normalization, and change parameters accordingly.

I've got a patch partially done that calls this "njsid", and it largely changes the JSObject property methods to accept njsids and pushes those normalization calls up the callgraph as appropriate.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Sorry, forgot to define "normalized": jsids have an int/string-atom duality. Those string atoms that actually represent smallish integers (i.e. "0") can be given to the engine and must be normalized into integer form to keep equivalence a simple operation.

Comment 2

[Nicholas Nethercote [inactive]]

Just to note:  js_CheckForStringIndex() is the function that currently does the normalizing.  It has this scary comment at the top:

/*
 * Convert string indexes that convert to int jsvals as ints to save memory.
 * Care must be taken to use this macro every time a property name is used, or
 * else double-sets, incorrect property cache misses, or other mistakes could
 * occur.
 */

As well as guaranteeing correctness, having a separate type may help efficiency slightly.  In at least one case we call js_CheckForStringIndex() twice on the same id in the same operation -- because js_GetPropertyHelper() calls it and also calls js_LookupPropertyWithFlags(), which also calls js_CheckForStringIndex().

I'm actually interested in pushing the normalization further up the call chain so it always happens as soon as jsids are created, thus avoiding the need for two different types.  But that might be difficult in some places, so this is a good interim step.

Comment 3

[Brendan Eich [:brendan]]

Types rule -- C drools (this was all C code in the old days, what else needs to be said).

/be

Comment 4

[Nicholas Nethercote [inactive]]

cdleary, can you post your partial patch up?  I'm thinking about returning to this because it would have prevented bug 645184.

Comment 5

[Nicholas Nethercote [inactive]]

Bug 645184 comment 22 has more details on when js_CheckForStringIndex is supposed to be used.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Waldo said that he'd get to this in the course of the object duality patches, so I'm WONFIXing this bug.