Closed Bug 687621 Opened 9 years ago Closed 9 years ago

More ObjectOps property-type splitting


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(3 files, 3 obsolete files)

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.
Attachment #561003 - Flags: review?(luke)
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.)
Attachment #561005 - Flags: review?(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.
Attached patch 1.5 - Address comment 2 (obsolete) — Splinter Review
How's this look?
Attachment #561305 - Flags: review?(luke)
Attachment #561005 - Flags: review?(luke) → review+
Attachment #561003 - Flags: review?(luke) → review+
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.
Attached patch 1.5, with SpecialId in jsclass.h (obsolete) — Splinter Review
Attachment #561305 - Attachment is obsolete: true
Attachment #561305 - Flags: review?(luke)
Attachment #561891 - Flags: review?(luke)
Attachment #561891 - Attachment is obsolete: true
Attachment #561891 - Flags: review?(luke)
Attachment #561892 - Flags: review?(luke)
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.
Attachment #561894 - Flags: review?(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?
Attachment #561892 - Flags: review?(luke) → review+
Attachment #561894 - Flags: review?(luke) → review+

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 :-)
Whiteboard: [inbound]
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.
Attachment #561005 - Attachment is obsolete: true
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.