[Webkit-unassigned] [Bug 53747] Add some dynamic annotations to JavaScriptCore/wtf

Fri Feb 4 11:32:44 PST 2011

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

--- Comment #11 from Timur Iskhodzhanov <timurrrr at google.com>  2011-02-04 11:32:43 PST ---
(In reply to comment #7)
> (From update of attachment 81172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81172&action=review

Mark, thanks for your review!
I've addressed most of your comments in my new patch though I have a couple of questions to ask.

So, the structure of my reply is:
1) Done, done, done
2) A couple of questions
3) A couple of comments I did not address on purpose

1.
> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:33
> > +#include "Platform.h"
> 
> This include should not be present.
Done.
I'm slightly worried that some files may not include config.h/Platform.h before DynamicAnnotations.h but it would violate your convention. 

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:71
> > +void WebKitAnnotateCondVarWait(const char *file, int line, const volatile void *cv, const volatile void *lock);
> 
> The WebKit prefix on these functions seems less than ideal given that they are in WTF.  A WTF prefix would be more in line with other straight C functions in WTF.
Done.

> The functions should be updated to match the WebKit style guide: Abbreviations shouldn’t be used in the names, the * for pointers should be next to the type name, and argument names shouldn’t be abbreviated.
Done except for abbrevations in the function names.
These dynamic analysis tools intercept annotations based on their names (disregarding the prefix), for example they do intercept
AnnotateCondVarSignal, WTFAnnotateCondVarSignal, WhatEverAnnotateCondVarSignal but they won't intercept AnnotateConditionVariableSignal.
In order to fix this we'll need to patch the tools upstream. This will take some extra time to roll new binaries everywhere and I'm not sure it's worth the effort, especially because developers are discouraged to use these functions directly.  

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:80
> > +#define ANNOTATE_HAPPENS_AFTER(obj) /* empty */
> 
> It’s not clear to me that the comments here add any value.
Done - I wrote a new comment for the whole bunch of these macros with some explanation. 

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:27
> > + * Author: Kostya Serebryany
> We don’t list author information in source files.  The version control system tracks this information.
After all, I'm not Kostya Serebryany :)
I'll talked to him and he removed that line upstream; I've updated the patch

2.
> > Source/JavaScriptCore/wtf/ThreadSafeShared.h:65
> > +#include <wtf/DynamicAnnotations.h>
> 
> Given that ThreadSafeShared.h is included by projects outside of JavaScriptCore (e.g., by WebCore) I suspect that this #include will break the Mac build unless you add a forwarding header.
Good catch!
I think build-webkit was succeeding locally on my MacBook but anyways I've added these files (similar to the forwarding headers for wtf/Atomics.h)
Should I also add these forwarding headers to some Makefiles/gyp/xcode/etc? If so - where?  

3.
A little background:
the files I'm trying to add are based on
http://code.google.com/p/data-race-test/source/browse/trunk/dynamic_annotations/dynamic_annotations.h
and
http://code.google.com/p/data-race-test/source/browse/trunk/dynamic_annotations/dynamic_annotations.c

We've been using those files at Google for more than two years already, including open-source projects like Chromium, NativeClient and more.

> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:35
> > +/* This file defines dynamic annotations for use with dynamic analysis
> 
> IMO the term “dynamic annotations” is far too generic to clearly communicate what it is that this file provides.  Is there some clearer term that can be used instead?
> > Source/JavaScriptCore/wtf/DynamicAnnotations.h:63
> > +#define ANNOTATE_HAPPENS_AFTER(obj) WebKitAnnotateCondVarWait(__FILE__, __LINE__, obj, 0)
> The naming of these two macros is confusing to me.  The names don’t appear to communicate what their purpose is.  Something happens before or after something else?  Can we come up with self-documenting names so that we don’t need to add multi-line comments at every use?
> > Source/JavaScriptCore/wtf/ThreadSafeShared.h:108
> > +        // See http://code.google.com/p/data-race-test/wiki/DynamicAnnotations#Reference_counting
> I can’t help but feel that if the macro names were clearer then we wouldn’t need comments like this where they’re used.
Yeah, naming of this stuff is a tough thing.
Personally, I totally agree with you.
The problem is that to really understand what these macros are doing you have to know the guts of data race detection tools which is not what we expect from the developers...
We've already tried renaming them a couple of times discussing them people working in this domain (including Mike Burrows, author of one of the first production-quality race detector) and what we have now is the best we could advance.
ANNOTATE_HAPPENS_BEFORE/AFTER is not confusing too much in my mind :) At least everyone understands that something happens-before something else and we give a link to a comprehensive explanation why it should be done that way.

Looking forward for your comments!

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