This file is big and ugly. Make it nonexistent and pretty. Or at least small and pretty.
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.
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.
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))
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.
(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 on attachment 50970 [details] Kill it cleaner. ok.
http://trac.webkit.org/changeset/56166