[webkit-reviews] review denied: [Bug 20422] Patch to allow custom memory allocation control : [Attachment 23132] Implements a new file: JavaScriptCore/wtf/New.h.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Paul Pedriana <ppedriana at gmail.com>'s
request for review:
Bug 20422: Patch to allow custom memory allocation control
https://bugs.webkit.org/show_bug.cgi?id=20422

Attachment 23132: Implements a new file: JavaScriptCore/wtf/New.h.
https://bugs.webkit.org/attachment.cgi?id=23132&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Seems good.

I think the term "Object" might be confusing here. I hope we can come up with
better names than deleteObject and deleteArray.

AllocBase seems like a reasonable name, but if we're going to be using it
everywhere, do we really have to use the abbreviation "alloc"? I'd prefer a
name that was a tiny bit more expressive about what it does exactly. Not that
I'm a fan of the prefix "fast" in "fastMalloc", but it would at least be
consistent to use that motif everywhere; we should consider that.

I think it's probably a mistake to name this <wtf/New.h> since there's a system
header called <new.h> on some platforms. We have trouble with
<WebCore/String.h> and we've had to name it PlatformString.h because of
conflict with <string.h> on some platforms.

The main thing I'd like to see that's not here is a way to detect if people use
new/delete directly and forget to use these functions. One of the advantages of
the current approach is that there's no room for error. You can't forget and
mismatch these. We would like a compile time error or a runtime error if
someone does delete instead of deleteObject or delete [] instead of
deleteArray. I'd like to come up with a way of doing that, even if only on
certain platforms. We can be creative and use macros if necessary. I also think
we consider putting debugging sentinels in versions when NDEBUG is not set so
we detect objects that are allocated with new/newObject and deleted with
delete[]/deleteArray or vice versa.

It's fine to use the underscore naming convention for things where we're trying
to match something in the std namespace. But I don't think that includes
integral_constant, true_type, and false_type. Unless perhaps those are coming
in a future standard?

+	 template <> struct has_trivial_constructor<float>		      :
public true_type{};
+	 template <> struct has_trivial_constructor<const float>	      :
public true_type{};

Why are these versions with "const" needed? Are they really needed? If so, then
why aren't versions with "volatile" and "const volatile" also needed?

HashTraits.h has a template called IsInteger. There are some differences in
idiom between this new code and that existing code that might be worth
resolving. And we may want to share with that template.

The has_trivial_constructor and has_trivial_destructor need to be specialized
for raw pointers too. What does "trivial" mean for a constructor? Does it
really mean you can use uninitialized memory for the type, as opposed to
needing zero-filled memory?

We normally put spaces inside empty braces. We normally try to avoid lining up
code, like the lined-up colons. It's "high maintenance" formatting that tends
to fall apart if you rename something.

+	     fastFree(p);  // We don't need to check for a null pointer; the
compiler does this.

I don't understand this comment. fastFree checks for 0 and quietly does
nothing. What does the compiler have to do with it?

+	 // This handles the case whereby T has a trivial ctor and a trivial
dtor.

I think you mean "wherein", not "whereby".

+		 T* const p = static_cast<T*>(fastMalloc(sizeof(T) * count));

We normally don't mark local variables const, even if we don't plan to modify
them. Is there a reason you're doing that here and not elsewhere?

+		 return reinterpret_cast<T*>(p);

Why is there a cast here (in NewArrayImpl<T, false, true>? I believe p already
has the right type.

+		 uint64_t* p =
static_cast<uint64_t*>(fastMalloc(sizeof(uint64_t) + (sizeof(T) * count)));

The use of uint64_t here is a little strange. Is that really what we want, even
on 32-bit platforms? Seems a bit inefficient. I guess you're trying to not
perturb the alignment so you're trying to chose an integer size that's big
enough everywhere. If so, then "unsigned long long" may be the best type to
use. I don't think explicitly specifying 64 bits is necessarily helpful.

I think a better approach would be to put the pointer into a char*, since that
has a special case to allow pointer aliasing. I believe the implementation you
have here would not compile with strict aliasing. We can't just cast directly
from uint64_t* to another arbitrary type. But we can cast from char* to two
different types. We should find an answer to this and test it. It's critical
that we continue to compile with the strict aliasing compiler option on.

If you were using the char technique, then we could advance by a size that
preserves alignment, while still using a 32-bit integer for the array size on
32-bit platforms, which is probably more efficient.

+	 if (p) { // As per the C++ standard, test for null pointer.

I think this comment is inaccurate. Clearly the C++ standard doesn't tell us
what a function template named deleteObject should do! What you're really
trying to say here is something more like this: "Since we intend to use this
wherever we'd use C++ delete, allow a null pointer as it does."

Since we plan to use this everywhere we might be using it in
performance-critical code paths. Perhaps we should consider adding version of
delete without a null check that instead assert? It might be good to save a
branch in the case of a trivial destructor. We could also consider a version of
fastFree without a null check for the same reason.

+		 if (p) { // As per the C++ standard, test for null pointer.
+		     // No need to destruct the objects in this case.
+		     fastFree(p);
+		 }

There's no need to add an extra check for null in this case since fastFree
already does this.

I think I made enough comments to justify review- for now. I'm really
interested in figuring out a strategy for using this in all the code that will
help us catch places where we forget to use it.