Bug 35941 - [V8] Use a struct instead of an int to indicate the dom wrapper tpye
Summary: [V8] Use a struct instead of an int to indicate the dom wrapper tpye
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks: 33477
  Show dependency treegraph
 
Reported: 2010-03-09 13:17 PST by Nate Chapin
Modified: 2010-03-10 14:53 PST (History)
4 users (show)

See Also:


Attachments
patch (78.49 KB, patch)
2010-03-09 13:57 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
patch with build file changes (79.45 KB, patch)
2010-03-09 14:14 PST, Nate Chapin
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2010-03-09 13:17:36 PST
Currently, when we wrap a WebCore object for use in V8, we insert 2 internal fields: a pointer to the WebCore object itself, and an integer indicating the V8ClassIndex::V8WrapperType.

Instead of the integer, we could use a struct that includes pointers to the appropriate versions of functions that WebCore might need for this object.  I believe the callbacks would be:

* A pointer to the appropriate GetTemplate function
* A pointer to a function that derefs the WebCore object (if it is refcounted, need in V8GCController.cpp)
* A pointer to a function that casts the WebCore object to an ActiveDOMObject if possible (needed for appropriate handling of active objects in DOMData.cpp)

This will allow us to remove any place that we're using a macro'ed switch statement based on the declarations in V8Index.h, as well as delete V8Index.cpp.

Once this change is in, I plan on working toward generating an equivalent to V8Index.h.
Comment 1 Nate Chapin 2010-03-09 13:57:14 PST
Created attachment 50343 [details]
patch

This appears to be essentially a no-op in terms of Dromaeo performance (within a fraction of 1% from previous results).
Comment 2 WebKit Review Bot 2010-03-09 14:12:09 PST
Attachment 50343 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/481007
Comment 3 Nate Chapin 2010-03-09 14:14:36 PST
Created attachment 50347 [details]
patch with build file changes
Comment 4 Dimitri Glazkov (Google) 2010-03-09 14:20:19 PST
This is better than peanut butter, jelly and bread put together. Anton, do you have any concerns with this patch?
Comment 5 Nate Chapin 2010-03-09 14:24:09 PST
(In reply to comment #4)
> This is better than peanut butter, jelly and bread put together. Anton, do you
> have any concerns with this patch?

I'm allergic to peanuts, you insensitive clod!
Comment 6 Dimitri Glazkov (Google) 2010-03-09 14:26:46 PST
(In reply to comment #5)
> (In reply to comment #4)
> > This is better than peanut butter, jelly and bread put together. Anton, do you
> > have any concerns with this patch?
> 
> I'm allergic to peanuts, you insensitive clod!

Darn it! I've gotta stop falling into this food sensitivity trap.
Comment 7 Dimitri Glazkov (Google) 2010-03-09 14:30:57 PST
What about sunflower butter? http://www.nutsonline.com/snacks/sunflowerseeds/butter.html?source=googlebase
Comment 8 Nate Chapin 2010-03-09 14:35:17 PST
(In reply to comment #7)
> What about sunflower butter?
> http://www.nutsonline.com/snacks/sunflowerseeds/butter.html?source=googlebase

I'm generally ok with seeds, so it's probably safe.  However, unlike the individual in their testimonial, I do not crave peanut butter nor any substitute.  It is the Kryptonite to my Superman, the CodeGeneratorV8.pm to my comprehensible codebase.
Comment 9 Ojan Vafai 2010-03-09 14:40:58 PST
Does this have any memory implications?
Comment 10 Nate Chapin 2010-03-09 14:48:41 PST
(In reply to comment #9)
> Does this have any memory implications?

AFAICT, no.  I ran chromium's memory_tests, which passed locally.  If you have any recommendations on other tests to run, I would be happy to hear them.

In terms of what is supposed be happening, instead of storing an integer, we're storing a pointer to a static struct that contents an integer and 3 function pointers.  Unless I screwed something up, that should be negligible.
Comment 11 anton muhin 2010-03-10 08:23:31 PST
LGTM.  And I like it.
Comment 12 Dimitri Glazkov (Google) 2010-03-10 08:26:39 PST
Comment on attachment 50347 [details]
patch with build file changes

beam me up, Scotty.
Comment 13 Nate Chapin 2010-03-10 14:53:21 PST
http://trac.webkit.org/changeset/55798