[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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #66825|review?                     |review-
               Flag|                            |




--- Comment #10 from Alexey Proskuryakov <ap at webkit.org>  2010-09-08 10:16:00 PST ---
(From update of attachment 66825)
> +        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.

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