Bug 33477 – [V8] V8Index should be generated

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

This file is big and ugly.  Make it nonexistent and pretty.  Or at least small and pretty.

[Nate Chapin]

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.

[Dimitri Glazkov (Google)]

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.

[anton muhin]

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))

[Nate Chapin]

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.

[anton muhin]

(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.

[Dimitri Glazkov (Google)]

Comment on attachment 50970 [details]
Kill it cleaner.

ok.

[Nate Chapin]

http://trac.webkit.org/changeset/56166