[Webkit-unassigned] [Bug 45134] Added statistics sampling and reporting for JavaScriptCore's RegisterFile and ExecutableAllocator classes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=45134





--- Comment #6 from Alexey Proskuryakov <ap at webkit.org>  2010-09-03 10:04:50 PST ---
(From update of attachment 66507)
+        Reviewed by NOBODY (OOPS!).

There should be a bug reference and title in ChangeLog.

> Added Runtime checks do not impact performance on SunSpider benchmark.

SunSpider is what we use for most performance measurements, but adding mutexes and spinlocks isn't likely to affect it. Locking a mutex is very fast if no one else is holding it, and only parallel execution that's going to be affected by lock contention.

        static void enableStats() { s_statsEnabled = true; }

I don't think that this function needs to be inline. "Statistics" should be spelled out, and the name (or at least a comment) should explain what kind of statistics is enabled. We already track lots of memory-related statistics, and this call doesn't affect most.

It's not necessarily clear that statistics are only reported for (de)allocations that happen after enabling. It's particularly unclear and even wrong since totalBytesCommitted() can be negative, but returns a size_t.

+    SpinLockHolder lockHolder(&spinlock);
+    s_totalBytesCommitted += byteCount;

Ideally, this would use a variant of atomicIncrement rather than locking, but the version we have in Atomics.h can only add 1.

We don't use SpinLock outside of allocator code. It's part of FastMalloc.

If a Mutex doesn't give adequate performance, we should probably consider moving SpinLock to a general WTF header.

+        static size_t totalBytesCommitted() { return s_totalBytesCommitted; }

Looking at a shared value also requires locking in general case. A size_t variable may be unaligned or longer than processor word, in which cases reading is not an atomic operation.

+    void addToStats(size_t byteCount)
+    {
+        s_totalBytesCommitted += byteCount;
+    }

Why is it OK to not do locking for FixedVMPoolAllocator? If another lock is expected to be held, I think that we should assert so.

> Index: WebKit/mac/ChangeLog

I don't know what exactly the requirements are, but I expected to see at least Windows and WebKit2 here, too.

++ (void)enableJavaScriptStatistics
+{
+    RegisterFile::enableStats();
+    ExecutableAllocator::enableStats();
+}

It's very unclear what statistics this method enables. Who is going to call it, and when?

This is only a low level review, someone more familiar with JSC and allocator code should tell if these are good things to report in general.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.