Bug 120523 – Allow WTF::Vector use memset for POD elements initialization

• Component: Web Template Framework
• Status: NEW

[Mikhail Pozdnyakov]

SSIA.

[Mikhail Pozdnyakov]

Created attachment 210095 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 210095 [details]
patch

canInitializeWithMemset is supposed to be ignored when needsInitialization is false. If that doesn't work, it's probably a bug elsewhere.

Have you observed any improvement from this patch?

[Mikhail Pozdnyakov]

(In reply to comment #2)
> (From update of attachment 210095 [details])
> canInitializeWithMemset is supposed to be ignored when needsInitialization is false. If that doesn't work, it's probably a bug elsewhere.
> 
> Have you observed any improvement from this patch?

This patch does not make any functional difference indeed at the moment because STD::Vector implementation is checking 'needsInitialization' flag first. 

But the trait class itself is now wrong because it's saying that memset
cannot(should not) be used for POD types, so the class is not 100% reliable. If
Vector implementation changes the error will turn up.

[Alexey Proskuryakov]

Even if Vector implementation changes, it still needs to respect needsInitialization, and thus ignore canInitializeWithMemset.

[Mikhail Pozdnyakov]

(In reply to comment #4)
> Even if Vector implementation changes, it still needs to respect needsInitialization, and thus ignore canInitializeWithMemset.

Why do we need 'canInitializeWithMemset' at all in this trait specialization than? Maybe it's worth removing it, if it will never be used anyway?

[Alexey Proskuryakov]

Having it simplifies Vector code (the value is passed to some functions and then ignored). If there is a way to remove the unneeded value without making Vector code more complicated, that would certainly be nice.

[Mikhail Pozdnyakov]

(In reply to comment #4)
> Even if Vector implementation changes, it still needs to respect needsInitialization, and thus ignore canInitializeWithMemset.

please consider following example:

Vector<pair<int, RefPtr<SomeClass> >

traits for it:

bool needsInitialization = FirstTraits::needsInitialization || SecondTraits::needsInitialization; -> true (as RefPtr requires it)
bool canInitializeWithMemset = FirstTraits::canInitializeWithMemset && SecondTraits::canInitializeWithMemset; -> false (should be true!)

So the patch actually might give a certain functional impact.
On the other hand I do not see any reason for having 'canInitializeWithMemset' disabled for POD types.

[Mikhail Pozdnyakov]

Created attachment 210353 [details]
alternate patch

Simplifies the whole VectorTraitsBase class.

[Darin Adler]

I don’t understand why “POD” means “OK to initialize with memset”. Those seem like two separate concepts.

[Mikhail Pozdnyakov]

(In reply to comment #9)
> I don’t understand why “POD” means “OK to initialize with memset”. Those seem like two separate concepts.

I could answer that at the moment POD type in WTF basically means just "numeric type or pointer" but here https://bugs.webkit.org/show_bug.cgi?id=120630 I'm trying to change it :)

However using memset for C++11 POD types initialization doesn't look wrong to me too: POD type is a type whose characteristics are supported by a data type in the C language - scalar types, struct or union (even though the expanded C++ grammar can be used in its declaration). And in C, it is a common idiom to zero out the memory for a struct using memset, the only particularity that I see here is that alignment bytes are set to zero value as well, but it should not be a trouble for us.

Am I missing anything?

[Mikhail Pozdnyakov]

any update on this?

[Darin Adler]

I’m still not 100% sure this is right, but seems OK to land it.

[WebKit Commit Bot]

Comment on attachment 210353 [details]
alternate patch

Rejecting attachment 210353 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 210353, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
y', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 2 diffs from patch file(s).
patching file Source/WTF/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WTF/wtf/VectorTraits.h
Hunk #1 FAILED at 35.
1 out of 1 hunk FAILED -- saving rejects to file Source/WTF/wtf/VectorTraits.h.rej

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Darin Adler']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6708429343162368

[Darin Adler]

Comment on attachment 210353 [details]
alternate patch

Maybe we should be using std::is_trivially_copyable instead of checking for POD?

[Darin Adler]

Comment on attachment 210353 [details]
alternate patch

On reflection, I think this is going in the wrong direction. Many of the VectorTraits questions can be answered by library functions such as std::is_default_constructible and such. A better direction would be to use those instead of vector traits, as long as we don’t lose any optimizations that way.