Bug 33477 - [V8] V8Index should be generated
: [V8] V8Index should be generated
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nate Chapin
:
Depends on: 34768 35941
Blocks: 32630
  Show dependency treegraph
 
Reported: 2010-01-11 10:45 PST by Nate Chapin
Modified: 2010-03-19 13:28 PDT (History)
3 users (show)

See Also:


Attachments
Kill V8Index. Kill it dead. (131.23 KB, patch)
2010-03-16 15:00 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Kill it cleaner. (129.99 KB, patch)
2010-03-17 16:21 PDT, 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-01-11 10:45:00 PST
This file is big and ugly.  Make it nonexistent and pretty.  Or at least small and pretty.
Comment 1 Nate Chapin 2010-03-16 15:00:33 PDT
Created attachment 50841 [details]
Kill V8Index.  Kill it dead.

Dromaeo perf for this patch was about a 0.3% improvement on my local box over 3 runs, so I'd say this qualifies as negligible for performance.

antonm: I had to modify the way we store boilerplate wrappers in V8DOMWindowShell, since this change will remove our monotonically increasing index.  It appears performance is comparable between storing them in a v8::Array and a wtf/HashMap, but I wanted to call this change to your attention in case it's a bad idea for some reason that I haven't noticed.
Comment 2 Dimitri Glazkov (Google) 2010-03-16 15:15:03 PDT
I have to come up with some special descriptive word for this patch. Hopefully something Nate is not allergic to. LGTM, but I want anton to look at the boilerplate stuff and how v8::Persistent<> members of the map are handled.
Comment 3 anton muhin 2010-03-17 01:56:02 PDT
Overall another very nice cleanup.  I am staring to wonder if aim is no lines of code in v8 bindings :)

Some comments:

1)WebCore/bindings/v8/V8DOMWindowShell.cpp:

143      ASSERT(m_context.IsEmpty() || !m_wrapperBoilerplates.IsEmpty());
 134     ASSERT(m_context.IsEmpty() || !m_wrapperBoilerplates.isEmpty());

These are not the same.  The first one checks handles---either m_context handle is empty (thus we haven't established it) or we must have m_wrapperBoilerplates handle (but array might still be empty).

The new one requires that there is at least one mapping which should be false.  I wonder if this ASSERT is triggered by our tests in debug builds.

2)
160      if (!m_wrapperBoilerplates.IsEmpty()) {
161  #ifndef NDEBUG
162          V8GCController::unregisterGlobalHandle(this, m_wrapperBoilerplates);
163  #endif
164          m_wrapperBoilerplates.Dispose();
165          m_wrapperBoilerplates.Clear();
166      }
 150     m_wrapperBoilerplates.clear();

Alas, that's not that easy---you leaving persistent handles and thus objects referenced by them were not garbage collected.  You need to iterate through handles and Dispose each handle.  Take a look at how maps from DOM objects to wrappers are managed.

Cosmetic:

 I wonder if in == &<Class>::info idiom we could just omit & (which looks slightly ugly to me).  We could just omit or make a static function out of it.  Another approach might <Class>::instanceOf method.  I resort to your judgment if any of those ideas seem reasonable.

Could/should we introduce helper inline method for reinterpret_cast<WrapperTypeInfo*>(v8::External::Unwrap(data))
Comment 4 Nate Chapin 2010-03-17 16:21:02 PDT
Created attachment 50970 [details]
Kill it cleaner.

I added to functions to WrapperTypeInfo: a static unwrap function and an equals() that takes a pointer and compares it to this.  That makes the comparisons slightly prettier.

Re: Garbage collection, I think I'm properly deleting everything now, but I feel like there's got to be a more efficient way to do it.  If I'm missing something obvious, please let me know.
Comment 5 anton muhin 2010-03-18 04:33:44 PDT
(In reply to comment #4)
> Created an attachment (id=50970) [details]
> Kill it cleaner.
> 
> I added to functions to WrapperTypeInfo: a static unwrap function and an
> equals() that takes a pointer and compares it to this.  That makes the
> comparisons slightly prettier.
> 
> Re: Garbage collection, I think I'm properly deleting everything now, but I
> feel like there's got to be a more efficient way to do it.  If I'm missing
> something obvious, please let me know.

Very nice, thanks a lot, Nate.

I cannot immediately see more efficient way to clean the stuff up and I hope that this path is never critical---looks like we shouldn't care that much about releasing things.

So I'd suggest to land it and watch perf botsm, hopefully nothing should pop up.
Comment 6 Dimitri Glazkov (Google) 2010-03-18 07:34:43 PDT
Comment on attachment 50970 [details]
Kill it cleaner.

ok.
Comment 7 Nate Chapin 2010-03-19 13:28:32 PDT
http://trac.webkit.org/changeset/56166