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


Attachments
Kill V8Index. Kill it dead. (131.23 KB, patch)
2010-03-16 15:00 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Kill it cleaner. (129.99 KB, patch)
2010-03-17 16:21 PST, Nate Chapin
dglazkov: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 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 From 2010-03-16 15:00:33 PST -------
Created an attachment (id=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 From 2010-03-16 15:15:03 PST -------
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 From 2010-03-17 01:56:02 PST -------
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 From 2010-03-17 16:21:02 PST -------
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.
------- Comment #5 From 2010-03-18 04:33:44 PST -------
(In reply to comment #4)
> Created an attachment (id=50970) [details] [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 From 2010-03-18 07:34:43 PST -------
(From update of attachment 50970 [details])
ok.
------- Comment #7 From 2010-03-19 13:28:32 PST -------
http://trac.webkit.org/changeset/56166