Bug 163255 - HasOwnPropertyCache needs to ref the UniquedStringImpls it sees
Summary: HasOwnPropertyCache needs to ref the UniquedStringImpls it sees
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-10 16:47 PDT by Saam Barati
Modified: 2016-10-15 13:35 PDT (History)
15 users (show)

See Also:


Attachments
patch (3.50 KB, patch)
2016-10-11 14:17 PDT, Saam Barati
ggaren: review+
Details | Formatted Diff | Diff
patch for landing (3.65 KB, patch)
2016-10-11 16:33 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-10-10 16:47:13 PDT
...
Comment 1 Saam Barati 2016-10-11 14:17:37 PDT
Created attachment 291295 [details]
patch
Comment 2 Saam Barati 2016-10-11 14:18:24 PDT
Comment on attachment 291295 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:62
> +        bitwise_cast<HasOwnPropertyCache*>(cache)->clear();

I'll make this a static_cast
Comment 3 WebKit Commit Bot 2016-10-11 14:19:23 PDT
Attachment 291295 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:116:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 JF Bastien 2016-10-11 14:20:13 PDT
Comment on attachment 291295 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:133
>          memset(this, 0, sizeof(Entry) * size);

Could you just placement new instead?
Comment 5 Geoffrey Garen 2016-10-11 14:33:01 PDT
Comment on attachment 291295 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:123
> +            bitwise_cast<Entry*>(this)[i].Entry::~Entry();

Pointer casting can use reinterpret_cast.
Comment 6 Saam Barati 2016-10-11 15:30:07 PDT
(In reply to comment #5)
> Comment on attachment 291295 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291295&action=review
> 
> > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:123
> > +            bitwise_cast<Entry*>(this)[i].Entry::~Entry();
> 
> Pointer casting can use reinterpret_cast.

I prefer bitwise_cast for this still because it does the sizeof check. Which prevents  e from making mistakes like this:
unsigned bar = ...;
Foo* ptr = bitwise_cast<Foo*>(bar);
Comment 7 Saam Barati 2016-10-11 16:33:18 PDT
Created attachment 291315 [details]
patch for landing
Comment 8 WebKit Commit Bot 2016-10-11 16:35:52 PDT
Attachment 291315 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:118:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 WebKit Commit Bot 2016-10-11 20:17:32 PDT
Comment on attachment 291315 [details]
patch for landing

Clearing flags on attachment: 291315

Committed r207186: <http://trac.webkit.org/changeset/207186>
Comment 10 WebKit Commit Bot 2016-10-11 20:17:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Carlos Garcia Campos 2016-10-12 02:32:43 PDT
This broke the build in several linux bots (GTK+, JSCOnly). Maybe something not supported by GCC 4.9?

https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/2318/steps/compile-webkit/logs/stdio
Comment 12 Csaba Osztrogonác 2016-10-12 02:51:10 PDT
(In reply to comment #11)
> This broke the build in several linux bots (GTK+, JSCOnly). Maybe something
> not supported by GCC 4.9?
> 
> https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/
> builds/2318/steps/compile-webkit/logs/stdio

I also noticed this error, adding Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) constructor fixed the build for me. I'm goning to land
it immediately.
Comment 13 Csaba Osztrogonác 2016-10-12 02:59:00 PDT
Fix landed in http://trac.webkit.org/changeset/207213.


(In reply to comment #11)
> This broke the build in several linux bots (GTK+, JSCOnly). Maybe something
> not supported by GCC 4.9?
> 
> https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/
> builds/2318/steps/compile-webkit/logs/stdio

But the question is why the GTK EWS was green? Do you have different
compiler on the EWS and buildbots? It would be great to keep them in sync.
Comment 14 Csaba Osztrogonác 2016-10-12 03:14:08 PDT
Additionally it was broken on Apple Windows and WinCairo bots too.

Is it a compiler bug in MSVC and GCC? Is C++11/14/17 allows the compiler to
generate Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) constructor?
Comment 15 Csaba Osztrogonác 2016-10-12 03:38:37 PDT
(In reply to comment #14)
> Additionally it was broken on Apple Windows and WinCairo bots too.
> 
> Is it a compiler bug in MSVC and GCC? Is C++11/14/17 allows the compiler to
> generate Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool)
> constructor?

I found the proper answer for this problem. In C++11 Entry isn't an aggregate
because of brace initializers of its members, but it is in C++14.

http://stackoverflow.com/questions/35891233/initializing-a-struct-with-aggregate-initialization-and-member-initializers

The problem here is that GCC 4.9 and VS 2015 
doesn't support NSDMIs for aggregates.
Comment 16 Carlos Garcia Campos 2016-10-12 04:04:35 PDT
(In reply to comment #13)
> Fix landed in http://trac.webkit.org/changeset/207213.
> 
> 
> (In reply to comment #11)
> > This broke the build in several linux bots (GTK+, JSCOnly). Maybe something
> > not supported by GCC 4.9?
> > 
> > https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/
> > builds/2318/steps/compile-webkit/logs/stdio
> 
> But the question is why the GTK EWS was green? Do you have different
> compiler on the EWS and buildbots?

Yes, unfortunately.

> It would be great to keep them in sync.

Agree
Comment 17 Darin Adler 2016-10-13 11:35:31 PDT
I believe the build fix added reference count churn:

+        Entry(RefPtr<UniquedStringImpl> impl, StructureID structureID, bool result)
+            : impl(impl)

This is not the same thing as the automatically generated behavior, which can do a move if passed a RefPtr&&.
Comment 18 Saam Barati 2016-10-13 11:41:03 PDT
(In reply to comment #17)
> I believe the build fix added reference count churn:
> 
> +        Entry(RefPtr<UniquedStringImpl> impl, StructureID structureID, bool
> result)
> +            : impl(impl)
> 
> This is not the same thing as the automatically generated behavior, which
> can do a move if passed a RefPtr&&.

Yeah I believe it should always be passed a RefPtr&&. This should be an easy fix.

Does the `StructName { ... }` syntax call the constructor?
Comment 19 Darin Adler 2016-10-13 12:06:11 PDT
(In reply to comment #18)
> Does the `StructName { ... }` syntax call the constructor?

Yes.
Comment 20 Saam Barati 2016-10-13 12:56:09 PDT
(In reply to comment #19)
> (In reply to comment #18)
> > Does the `StructName { ... }` syntax call the constructor?
> 
> Yes.

Interesting. I had thought the "{ ... }" just individually constructs each field.
Do you know what the rules are for it calling a constructor? Must there be a constructor that matches the types? Like
StructName{ T1,...,TN }
Calls constructor of type
ConstructorTye(T1, ..., TN)
?
(Ignoring type conversions, etc)
Comment 21 Darin Adler 2016-10-13 13:05:57 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Does the `StructName { ... }` syntax call the constructor?
> > 
> > Yes.
> 
> Interesting. I had thought the "{ ... }" just individually constructs each
> field.

Only if there is no constructor.

> Do you know what the rules are for it calling a constructor?

An oversimplification: direct initialization of the data members with that syntax works only if there are no constructors. For the full story, C++ reference material recommended. I normally use cppreference.com to look things up.

 Must there be a
> constructor that matches the types? Like
> StructName{ T1,...,TN }
> Calls constructor of type
> ConstructorTye(T1, ..., TN)
> ?
> (Ignoring type conversions, etc)

Yes. In fact, the ClassName { } syntax does not allow many type conversions that the ClassName ( ) syntax performs, which is another benefit of using it.
Comment 22 Saam Barati 2016-10-15 13:33:06 PDT
I've opened this bug to go back to move semantics:
https://bugs.webkit.org/show_bug.cgi?id=163490
Comment 23 Saam Barati 2016-10-15 13:35:10 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #18)
> > > > Does the `StructName { ... }` syntax call the constructor?
> > > 
> > > Yes.
> > 
> > Interesting. I had thought the "{ ... }" just individually constructs each
> > field.
> 
> Only if there is no constructor.
> 
> > Do you know what the rules are for it calling a constructor?
> 
> An oversimplification: direct initialization of the data members with that
> syntax works only if there are no constructors. For the full story, C++
> reference material recommended. I normally use cppreference.com to look
> things up.
Interesting. I'll read about this. It's surprising to me because I had just assumed the syntax was more or less shorthand for initializing each field. That was a bad assumption on my part. 
> 
>  Must there be a
> > constructor that matches the types? Like
> > StructName{ T1,...,TN }
> > Calls constructor of type
> > ConstructorTye(T1, ..., TN)
> > ?
> > (Ignoring type conversions, etc)
> 
> Yes. In fact, the ClassName { } syntax does not allow many type conversions
> that the ClassName ( ) syntax performs, which is another benefit of using it.