[webkit-reviews] review denied: [Bug 45134] Added statistics sampling and reporting for JavaScriptCore's RegisterFile and ExecutableAllocator classes : [Attachment 66825] Revised patch w/ use of Mutex in RegisterFile and runtime enabling/disabling removed

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied John Therrell
<jtherrell at apple.com>'s request for review:
Bug 45134: Added statistics sampling and reporting for JavaScriptCore's
RegisterFile and ExecutableAllocator classes
https://bugs.webkit.org/show_bug.cgi?id=45134

Attachment 66825: Revised patch w/ use of Mutex in RegisterFile and runtime
enabling/disabling removed
https://bugs.webkit.org/attachment.cgi?id=66825&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   static size_t s_totalBytesCommitted;
> +	   Mutex mutex;

"mutex" is not a good name for the new member variable. It doesn't convey any
meaning, and it should start with "m_". I'd suggest "m_statisticsMutex".

But most importantly, the variable it's protecting is static, so there should
be only one instance of the mutex. Locking one mutex has no effect on other
instances, so several RegisterFile can currently write into
s_totalBytesCommitted simultaneously, which is a problem.

> +	   ASSERT(IsHeld(spinlock));

IsHeld() is a member function, so this should be spinlock.IsHeld().

Please make sure that this patch doesn't break debug builds. Please also run
tests in debug builds, so that assertions would have a chance to fire.

> +    void addToStatistics(size_t byteCount)
> +    {
> +	   s_totalBytesCommitted += byteCount;
> +    }

The ASSERT() should be where we actually need it to hold - so it's better to
put it here, and not at call site.

+size_t RegisterFile::registerFileStatistics()
+{
+    return s_totalBytesCommitted;
+}
...
> +size_t FixedVMPoolAllocator::fixedVMPoolAllocatorStatistics()
> +{
> +    return s_totalBytesCommitted;
> +}

These should also use locking. Otherwise, we may be reading while another
thread writes, and get a partially overwritten value.

> +size_t ExecutableAllocator::executableAllocatorStatistics()
> +{
> +    return FixedVMPoolAllocator::fixedVMPoolAllocatorStatistics();
> +}

I don't see why two functions are needed. Isn't just the static version
sufficient?

> +    size_t jscStackBytes = RegisterFile::registerFileStatistics();
> +    size_t jscJITBytes =
ExecutableAllocator::executableAllocatorStatistics();

It seems like these function names can be further improved. "Statistics" is
much less informative than what you have e.g. for these variable names.