Last Comment Bug 687621 - More ObjectOps property-type splitting
: More ObjectOps property-type splitting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-19 13:27 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-09-23 07:43 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1 - Introduce two further property-based API splits, in the ObjectOps struct (43.92 KB, patch)
2011-09-19 13:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
2 - Change getProperty to take a PropertyName*, convert users as appropriate (47.58 KB, patch)
2011-09-19 13:31 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
1.5 - Address comment 2 (39.26 KB, patch)
2011-09-20 14:34 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
1.5, with SpecialId in jsclass.h (2.15 KB, patch)
2011-09-22 14:45 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
1.5, with SpecialId in jsclass.h (29.13 KB, patch)
2011-09-22 14:46 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
2 - Change getProperty to take a PropertyName*, convert users as appropriate (48.10 KB, patch)
2011-09-22 14:48 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-19 13:27:06 PDT
Created attachment 561003 [details] [diff] [review]
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] (remove +bmo to email) 2011-09-19 13:31:13 PDT
Created attachment 561005 [details] [diff] [review]
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] 2011-09-20 09:56:42 PDT
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] (remove +bmo to email) 2011-09-20 14:34:55 PDT
Created attachment 561305 [details] [diff] [review]
1.5 - Address comment 2

How's this look?
Comment 4 Luke Wagner [:luke] 2011-09-20 17:37:34 PDT
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] (remove +bmo to email) 2011-09-22 14:45:27 PDT
Created attachment 561891 [details] [diff] [review]
1.5, with SpecialId in jsclass.h
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-22 14:46:05 PDT
Created attachment 561892 [details] [diff] [review]
1.5, with SpecialId in jsclass.h
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-22 14:48:07 PDT
Created attachment 561894 [details] [diff] [review]
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] 2011-09-22 15:05:33 PDT
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 10 Ed Morley [:emorley] 2011-09-23 04:48:03 PDT
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] (remove +bmo to email) 2011-09-23 07:43:02 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.