[webkit-reviews] review denied: [Bug 27980] Give an ability to WebKit to free statically allocated pointers before quit : [Attachment 39334] Sixth patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 23 10:23:06 PDT 2009
Darin Adler <darin at apple.com> has denied Zoltan Herczeg
<zherczeg at inf.u-szeged.hu>'s request for review:
Bug 27980: Give an ability to WebKit to free statically allocated pointers
before quit
https://bugs.webkit.org/show_bug.cgi?id=27980
Attachment 39334: Sixth patch
https://bugs.webkit.org/attachment.cgi?id=39334&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
I really dislike the direction this is going. Adding many new classes and lots
of complexity. And lots of code that won't compile in most platforms, so will
cause platform-specific build failures.
I don't think this is worth it.
> +#include "wtf/GlobalPtr.h"
Includes of wtf should be:
#include <wtf/GlobalPtr.h>
I think GlobalPtr should probably be named GlobalOwnPtr and have an interface
as close as possible to the OwnPtr class, since it's a sort of hybrid between
OwnPtr and a raw pointer.
> +WTF::GlobalPtr<JSGlobalData> sharedInstancePtr;
Items in the WTF namespace are supposed to have "using WTF::GlobalPtr" in the
header and not be qualified by WTF explicitly in code.
I don't understand why it's better to move the actual pointer outside of a
function. It still seems a good programming style to scope a global variable
inside a function when that's possible.
> +#include "wtf/FastMalloc.h"
> +#include "wtf/StaticPtrBase.h"
Need to use angle brackets for these includes.
> +// All HashTables are static and generated by a script (see the comment in
the header file)
Need a period at the end of this sentence.
> +struct HashTableDeleter {
> +public:
No need for "public" since this is a struct. A struct where everything is
public should omit the public: part.
> + WTF::StaticPtrBase m_base;
No WTF:: prefix.
> + HashTable* m_hashTable;
> +
> + static void free(void* ptr)
> + {
> +
reinterpret_cast<HashTableDeleter*>(ptr)->m_hashTable->deleteTable();
This should be a static_cast, not reinterpret_cast.
> +#if ENABLE(FREE_STATIC_PTRS)
> + HashTableDeleter* deleter =
reinterpret_cast<HashTableDeleter*>(fastMalloc(sizeof(HashTableDeleter)));
This should be a static_cast, not a reinterpret_cast.
Why use fastMalloc here and not new?
> +// The classes here are defined as structs to avoid
> +// the call of C++ constructors and destructors
This comment and technique is incorrect. Using the keyword "struct" instead of
"class" has no effect on the presence of C++ constructors and destructors.
There are only two differences between class and struct:
1) Default visibility is public for struct, private for class.
2) Visual Studio treats struct and class as distinct for type identity and
name mangling. This goes against the C++ standard, but requires that we be
consistent if we want compatibility with that compiler.
Otherwise, they are identical and struct vs. class has no effect on whether
something has a non-trivial constructor and destructor.
> + // Has NO C++ constructor
The terminology here is strange. Why say "C++" when all this code is C++?
The comments in this file are not clarifying what this class is for. They need
to say that it's intended to be used for global data. And to satisfy our
requirement that there be no global destructors in the WebKit projects if
FREE_STATIC_PTRS is not set, they must have no non-trivial destructor. And to
satisfy our requirement that there be no global constructors in the WebKit
projects they must have no non-trivial constructor.
Some of that needs to be said.
> + void set(PtrType);
> + void setPtr(PtrType);
I have no idea how set differs from setPtr. Does not seem to be good design to
have both, but perhaps there is some real difference here. If so we need to use
a more descriptive name. For example, in other places we use terms like
"adopt". Somehow I guess setPtr means "take this pointer value, but not
ownership". And if so, it seems very strange design to have a pointer that
sometimes owns what it points to, and other times does not. Seems like overkill
to have that. If it's not going to own something, then we can use a raw
pointer.
> + void operator=(PtrType);
And also have an = operator. Why have all three of these?
> + void deletePtr();
What is this for?
> +#if ENABLE(FREE_STATIC_PTRS)
> + StaticPtrBase m_base;
> +#endif
This makes every global pointer 8 bytes. Is this really the best we can do?
> + delete reinterpret_cast<GlobalPtr<T>*>(ptr)->get();
This should be a static_cast, not reinterpret_cast.
> + void setFree(StaticPtrBaseFree func)
The type name StaticPtrBaseFree is not so good, because "free" is a verb or
adjective, not a noun. Types normally should be nouns.
Instead of "func" please use "function". There's no need to abbreviate. Except
in certain cases where something needs to be
> + // Detect memory leaks
> + ASSERT(!m_ptr || (m_ptr == value));
The comment here is confusing. How does this comment "detect memory leaks"? I
think it's strange to have this function allow you to set the same value
multiple times, given that the set operation is really "give ownership". Why do
we need that flexibility? I think the set function should probably be named
"adopt" if it is taking ownership.
> +template<typename T>
> +ALWAYS_INLINE void GlobalPtr<T>::operator=(PtrType value)
Do we really need this? I think assignment is confusing when it transfers
ownership of a raw pointer, so it would be best to leave this out unless it's
really needed.
> +// Wrapper class for arrays, which assigned to a static variable
> +// By default, it behaves like a simple C++ array without performance
overhead
These comments need periods.
The comment leaves out what really matters, which is what this class is for! It
says that by default it works as a simple array, but what does it do when not
default?
> + delete[] reinterpret_cast<LocalStaticArrayPtr<T>*>(ptr)->m_ptr;
This should be static_cast.
> + while (current) {
> + // Some pointers (i.e: HashTableDeleter) frees themself
"Some pointers free themselves." would be correct grammar.
The Latin abbreviation "i.e." means "in other words". I think you mean "for
example" here and that should be written either as "for example" or as "e.g."
if you want to use the Latin abbreviation.
The comment is *almost* helpful, but is just a bit too obscure. The comment
needs to be more explicit and say clearly that we follow the next pointer
before calling free because in some cases the free function will delete the
item including the next pointer.
> + ptr->m_free(reinterpret_cast<void*>(ptr));
There is no need for this cast at all.
> + // Perhaps a double linked list may be suitable if remove() called
frequently
This is a confusing comment, because it does not say what the tradeoff is. Why
aren't we using a doubly-linked list now? I suggest we not even include the
remove() function unless we need it. And if we need it we should have the
doubly-linked list from the start, unless there's a clear reason we should not.
> + StaticPtrBase* current = head();
> + if (current == this) {
> + m_head = current->next();
> + return;
> + }
> +
> + while (current->next() != this) {
> + current = current->next();
> + ASSERT(current);
> + }
> + current->m_next = next();
> +}
It's a little strange to use next() to get the pointer and then m_next to set
it. There's no abstraction there, just slight confusion, by having some of
each. There are cleaner ways to write the linked list algorithms that require
fewer special cases and don't fetch current->next() multiple times, but this is
probably OK as-is.
> + // Methods
> + void append();
> + void remove();
This comment is not good. First of all, "methods" is not a C++ term, and
second, what does that comment add?
> + static StaticPtrBase* m_head;
> + StaticPtrBase* m_next;
> + StaticPtrBaseFree m_free;
> +
> + static inline StaticPtrBase* head() { return m_head; }
> + inline StaticPtrBase* next() { return m_next; }
The use of inline here does no good -- functions defined inside a C++ class are
always treated as if they had "inline" specified, so there's no point in saying
it explicitly.
It doesn't make sense to have both public data members and public function
members that get at that same data. Either the data should be private, or the
functions should be omitted.
> +#if !PLATFORM(QT)
> delete data;
> +#endif
Why is Qt a special case here? This is very confusing!
> --- a/WebCore/WebCore.pro
> +++ b/WebCore/WebCore.pro
> @@ -1044,6 +1044,7 @@ SOURCES += \
> html/HTMLViewSourceDocument.cpp \
> html/ImageData.cpp \
> html/PreloadScanner.cpp \
> + html/TimeRanges.cpp \
> html/ValidityState.cpp \
> inspector/ConsoleMessage.cpp \
> inspector/InspectorBackend.cpp \
> @@ -2629,7 +2630,6 @@ contains(DEFINES, ENABLE_VIDEO=1) {
> html/HTMLMediaElement.cpp \
> html/HTMLSourceElement.cpp \
> html/HTMLVideoElement.cpp \
> - html/TimeRanges.cpp \
> platform/graphics/MediaPlayer.cpp \
> rendering/MediaControlElements.cpp \
> rendering/RenderVideo.cpp \
Can you make the above change in a separate check-in first?
> +#if !ENABLE(FREE_STATIC_PTRS)
> ~IdentifierRep()
> {
> // IdentifierReps should never be deleted.
> ASSERT_NOT_REACHED();
> }
> +#endif
This is a good example of something that's probably wrong. I think there is
storage to be freed here. Simply changing things so it compiles means that we
have a storage leak in the FREE_STATIC_PTRS case.
This patch is way too big, doing too much at once. It is easy to miss mistakes
like this in a patch of this size.
> static PassRefPtr<CSSInitialValue> createExplicit()
> {
> - static CSSInitialValue* explicitValue = new CSSInitialValue(false);
> - return explicitValue;
> + DEFINE_STATIC_LOCAL(CSSInitialValue, explicitValue, (false));
> + return &explicitValue;
> }
> static PassRefPtr<CSSInitialValue> createImplicit()
> {
> - static CSSInitialValue* explicitValue = new CSSInitialValue(true);
> - return explicitValue;
> + DEFINE_STATIC_LOCAL(CSSInitialValue, explicitValue, (true));
> + return &explicitValue;
> }
These kinds of changes should go in first, in a separate patch.
> - delete defaultStyle;
> - delete simpleDefaultStyleSheet;
> + defaultStyle.deletePtr();
> + simpleDefaultStyleSheet.deletePtr();
> defaultStyle = new CSSRuleSet;
> simpleDefaultStyleSheet = 0;
This is a bad idiom. The global pointer is a variation on OwnPtr, and the right
idiom there is to delete things automatically when assigning a new value.
Having an explicit deletePtr makes it too easy to use it wrong.
It's extremely important to have a patch that creates the mechanism separate
from a patch that deploys it in so many places. By making a single large patch
you're making it very hard to review, and there are lots of things you will
have to change now based on my feedback.
I can't read the rest of this right now.
More information about the webkit-reviews
mailing list