687621 - More ObjectOps property-type splitting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 1 - Introduce two further property-based API splits, in the ObjectOps struct

We have getProperty and getElement now, the former taking jsid, the latter taking an index.  We need a form that takes a property name, that can't be an element, i.e. js::PropertyName*.  That can't be jsid-based, because jsid could be either right now.  But jsid also encompasses special-valued jsids, distinct from property names and from indexes.  So we need a third form to fill out all the space jsid currently represents.  And in the short term, a fourth form that's jsid-based, which I'll call "generic", can help ease the transition to the narrower interfaces.

So in summary:

* add a third split to the property-based APIs, for use for "special" jsids
* add a fourth jsid-based split to the property-based APIs, for use in the
  interim while transitioning to the narrower APIs
* start to change the *Property APIs to use js::PropertyName, not jsid

This bug will fully cover the first two bullets.  I'll throw in one patch, for the getProperty APIs, making a start at the third bullet -- just enough to show things work.  There are enough that will need changing that it seems better to put them off to separate bugs, for the most part.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: 2 - Change getProperty to take a PropertyName*, convert users as appropriate

The obvious users, I converted to getProperty.  The non-obvious ones I left with getGeneric.  It doesn't seem like a good idea to spend time fixing all of the non-obvious ones, because some of them I think will go away with broader refactoring.  I think it'll be more obvious what the form of that will be, and what exactly it'll look like, when all the ObjectOps methods are rewritten to be like this patch makes get*.  (There's also a fair chance that when I get to the actual sparse/dense storage change, much of that code will melt away or change so substantially that I'd be wasting time fixing it now.)

Comment 2

[Luke Wagner [:luke]]

From IRL, two requests:

1. give the "special" property lookup paths a special new variant type that is a proper subset of jsid.
2. A big beefy comment somewhere explaining the { property-name, element, special } partitioning (and the associated invariants) of the public jsid.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 1.5 - Address comment 2

How's this look?

Comment 4

[Luke Wagner [:luke]]

This is a reasonable transition strategy; it's good to land things early and all, but I hope the *Generic ops don't last too long.

I'd like to make jsspecial private (in the to-be-created private jsclass.h header) and call it js::SpecialId or something.  It seems like this should be possible without duplicating functionality by implementing js::SpecialId operations in terms of JSID_ operations.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: 1.5, with SpecialId in jsclass.h

Comment 6

[Jeff Walden [:Waldo]]

Attachment: 1.5, with SpecialId in jsclass.h

Comment 7

[Jeff Walden [:Waldo]]

Attachment: 2 - Change getProperty to take a PropertyName*, convert users as appropriate

There were enough changes here to deal with js::SpecialId that it seems worth a second look, just to be safe.

I don't like the way the ValueIsSpecial bit works.  It seems better to refactor without changing behavior, tho.  There's probably something vaguely more sensible to do here, but now isn't the time to do it.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 561892 [details] [diff] [review]
1.5, with SpecialId in jsclass.h

Review of attachment 561892 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent, this is exactly what I was hoping for.

::: js/src/jsclass.h
@@ +76,5 @@
> +
> +  public:
> +    SpecialId() : bits(TYPE_VOID) { }
> +
> +    jsid toId() const {

For regularity with the other JSID_* operations, can this un-member-ed (put next to its bretheren below) and renamed to SPECIALID_TO_JSID?

Comment 9

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff35c22fc423
https://hg.mozilla.org/integration/mozilla-inbound/rev/b117f5ff61db
https://hg.mozilla.org/integration/mozilla-inbound/rev/faa84974073b

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ff35c22fc423
https://hg.mozilla.org/mozilla-central/rev/b117f5ff61db
https://hg.mozilla.org/mozilla-central/rev/faa84974073b

There are four attachments but three commits, is there one to come, or is attachment #561005 [details] [diff] [review] meant to be obsolete?

Leaving open for now, please close if indeed finished here. Thanks :-)

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 561005 [details] [diff] [review]
2 - Change getProperty to take a PropertyName*, convert users as appropriate

Hmm, yeah, I forgot to mark this obsolete.