Bug 165741 - MarkedBlock::marksConveyLivenessDuringMarking should take into account collection scope
Summary: MarkedBlock::marksConveyLivenessDuringMarking should take into account collec...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
: 165735 (view as bug list)
Depends on: 165853
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-10 23:09 PST by Filip Pizlo
Modified: 2016-12-14 10:39 PST (History)
9 users (show)

See Also:


Attachments
the patch (56.08 KB, patch)
2016-12-10 23:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (56.23 KB, patch)
2016-12-11 16:26 PST, Filip Pizlo
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-12-10 23:09:26 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2016-12-10 23:35:32 PST
Created attachment 296857 [details]
the patch
Comment 2 WebKit Commit Bot 2016-12-10 23:37:27 PST
Attachment 296857 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/DataLog.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/LockedPrintStream.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/RecursiveLockAdapter.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/PrintStream.h:287:  The parameter name "tuple" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Filip Pizlo 2016-12-11 16:26:55 PST
Created attachment 296876 [details]
the patch
Comment 4 WebKit Commit Bot 2016-12-11 16:29:26 PST
Attachment 296876 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/DataLog.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/LockedPrintStream.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/RecursiveLockAdapter.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
ERROR: Source/WTF/wtf/PrintStream.h:287:  The parameter name "tuple" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2016-12-11 16:45:24 PST
Comment on attachment 296876 [details]
the patch

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

The JSC bits and WTF bits look good to me. Perhaps it's worth allowing other people to also comment on the WTF portion before I r+

> Source/JavaScriptCore/runtime/Options.cpp:339
> +    Options::useConcurrentGC() = true;

Oops?

> Source/WTF/wtf/PrintStream.h:304
> +template<unsigned index, typename... Types>
> +struct FormatImplUnpacker {
> +    template<typename... Args>
> +    static void unpack(PrintStream& out, const std::tuple<Types...>& tuple, const Args&... values);
> +};
> +    
> +template<typename... Types>
> +struct FormatImplUnpacker<0, Types...> {
> +    template<typename... Args>
> +    static void unpack(PrintStream& out, const std::tuple<Types...>& tuple, const Args&... values)
> +    {
> +        out.printfVariableFormat(std::get<0>(tuple), values...);
> +    }
> +};
> +    
> +template<unsigned index, typename... Types>
> +template<typename... Args>
> +void FormatImplUnpacker<index, Types...>::unpack(PrintStream& out, const std::tuple<Types...>& tuple, const Args&... values)
> +{
> +    FormatImplUnpacker<index - 1, Types...>::unpack(out, tuple, std::get<index>(tuple), values...);
> +}

I wonder if this can be simplified if we used std::index_sequence.

> Source/WTF/wtf/RecursiveLockAdapter.h:50
> +        m_recursionCount = 1;

Is it worth asserting m_recursionCount == 0 before this store?

> Source/WTF/wtf/RecursiveLockAdapter.h:73
> +        m_recursionCount = 1;

Ditto?
Comment 6 Filip Pizlo 2016-12-11 16:46:02 PST
*** Bug 165735 has been marked as a duplicate of this bug. ***
Comment 7 Saam Barati 2016-12-11 16:47:26 PST
Comment on attachment 296876 [details]
the patch

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

>> Source/WTF/wtf/PrintStream.h:304
>> +}
> 
> I wonder if this can be simplified if we used std::index_sequence.

Actually, I thought about this more and forgot to delete this comment. I think it's ok the way it is.
Comment 8 Filip Pizlo 2016-12-11 16:48:36 PST
(In reply to comment #5)
> Comment on attachment 296876 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296876&action=review
> 
> The JSC bits and WTF bits look good to me. Perhaps it's worth allowing other
> people to also comment on the WTF portion before I r+
> 
> > Source/JavaScriptCore/runtime/Options.cpp:339
> > +    Options::useConcurrentGC() = true;
> 
> Oops?

Yeah, I'll remove this before landing.

> 
> > Source/WTF/wtf/PrintStream.h:304
> > +template<unsigned index, typename... Types>
> > +struct FormatImplUnpacker {
> > +    template<typename... Args>
> > +    static void unpack(PrintStream& out, const std::tuple<Types...>& tuple, const Args&... values);
> > +};
> > +    
> > +template<typename... Types>
> > +struct FormatImplUnpacker<0, Types...> {
> > +    template<typename... Args>
> > +    static void unpack(PrintStream& out, const std::tuple<Types...>& tuple, const Args&... values)
> > +    {
> > +        out.printfVariableFormat(std::get<0>(tuple), values...);
> > +    }
> > +};
> > +    
> > +template<unsigned index, typename... Types>
> > +template<typename... Args>
> > +void FormatImplUnpacker<index, Types...>::unpack(PrintStream& out, const std::tuple<Types...>& tuple, const Args&... values)
> > +{
> > +    FormatImplUnpacker<index - 1, Types...>::unpack(out, tuple, std::get<index>(tuple), values...);
> > +}
> 
> I wonder if this can be simplified if we used std::index_sequence.

I did it this way because I remembered doing it this way before.  What is index_sequence?

> 
> > Source/WTF/wtf/RecursiveLockAdapter.h:50
> > +        m_recursionCount = 1;
> 
> Is it worth asserting m_recursionCount == 0 before this store?
> 
> > Source/WTF/wtf/RecursiveLockAdapter.h:73
> > +        m_recursionCount = 1;
> 
> Ditto?

Added both assertions.
Comment 9 Saam Barati 2016-12-11 16:51:58 PST
Comment on attachment 296876 [details]
the patch

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

>>>> Source/WTF/wtf/PrintStream.h:304
>>>> +}
>>> 
>>> I wonder if this can be simplified if we used std::index_sequence.
>> 
>> Actually, I thought about this more and forgot to delete this comment. I think it's ok the way it is.
> 
> I did it this way because I remembered doing it this way before.  What is index_sequence?

This was the wrong name for it. I meant integer_sequence:
http://en.cppreference.com/w/cpp/utility/integer_sequence

I think it's easy enough to read this way, but I think integer_sequence could reduce the number of lines of code.
Comment 10 Saam Barati 2016-12-11 16:54:43 PST
Comment on attachment 296876 [details]
the patch

r=me

If others have concerns about the new logging code, they can be addressed later.
Comment 11 Filip Pizlo 2016-12-11 16:54:58 PST
CC'd some folks since this adds more to WTF.
Comment 12 Filip Pizlo 2016-12-11 16:58:39 PST
I'm going to wait for EWS.
Comment 13 Darin Adler 2016-12-11 17:24:27 PST
Comment on attachment 296876 [details]
the patch

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

> Source/WTF/wtf/LockedPrintStream.h:27
> +#ifndef LockedPrintStream_h
> +#define LockedPrintStream_h

Please use #pragma once instead of header guards in new files in WebKit.

> Source/WTF/wtf/PrintStream.cpp:50
> +#pragma clang diagnostic ignored "-Wformat-nonliteral"

Can we use WTF_ATTRIBUTE_PRINTF(2, 3) when declaring this function instead of doing this?

> Source/WTF/wtf/RecursiveLockAdapter.h:27
> +#ifndef WTF_RecursiveLockAdapter_h
> +#define WTF_RecursiveLockAdapter_h

#pragma once
Comment 14 Filip Pizlo 2016-12-11 17:29:42 PST
(In reply to comment #13)
> Comment on attachment 296876 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296876&action=review
> 
> > Source/WTF/wtf/LockedPrintStream.h:27
> > +#ifndef LockedPrintStream_h
> > +#define LockedPrintStream_h
> 
> Please use #pragma once instead of header guards in new files in WebKit.

Has this worked for you in WTF?  I use #pragma once elsewhere, but in WTF it often causes problems downstream.  I'm happy to try #pragma once and land a follow-up fix with it, if it builds.

> 
> > Source/WTF/wtf/PrintStream.cpp:50
> > +#pragma clang diagnostic ignored "-Wformat-nonliteral"
> 
> Can we use WTF_ATTRIBUTE_PRINTF(2, 3) when declaring this function instead
> of doing this?

Oh man, this was weird - because format() uses C++ variadic templates to pass the argument list, clang's format warnings go haywire. Clang's warnings are brutal in this regard: they refuse to let you pass a non-literal to a format function unless you're also passing a va_list (and not filling in the ...), but since I'm using variadic templates to fill the ..., the compiler somehow fails to see that the format string is really a literal.

I worked around this by introducing the printfVariableFormat trick, which allows the variadic template stuff to call something that doesn't warn.  I then hid the warning inside printfVariableFormat.

> 
> > Source/WTF/wtf/RecursiveLockAdapter.h:27
> > +#ifndef WTF_RecursiveLockAdapter_h
> > +#define WTF_RecursiveLockAdapter_h
> 
> #pragma once

I'll try it!  I'll land it if it builds!
Comment 15 Filip Pizlo 2016-12-11 17:30:29 PST
Landed in https://trac.webkit.org/changeset/209691

I'll land follow-up fixes soon.
Comment 16 Darin Adler 2016-12-11 17:53:05 PST
Comment on attachment 296876 [details]
the patch

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

>>> Source/WTF/wtf/LockedPrintStream.h:27
>>> +#define LockedPrintStream_h
>> 
>> Please use #pragma once instead of header guards in new files in WebKit.
> 
> Has this worked for you in WTF?  I use #pragma once elsewhere, but in WTF it often causes problems downstream.  I'm happy to try #pragma once and land a follow-up fix with it, if it builds.

Yes, there are 42 files in WTF that are already using it.

I have seen some problems where the Windows build was depending on use of the same header at two different paths. And another problem with some kind of cyclic header inclusion. But most places it has been working well.

>>> Source/WTF/wtf/PrintStream.cpp:50
>>> +#pragma clang diagnostic ignored "-Wformat-nonliteral"
>> 
>> Can we use WTF_ATTRIBUTE_PRINTF(2, 3) when declaring this function instead of doing this?
> 
> Oh man, this was weird - because format() uses C++ variadic templates to pass the argument list, clang's format warnings go haywire. Clang's warnings are brutal in this regard: they refuse to let you pass a non-literal to a format function unless you're also passing a va_list (and not filling in the ...), but since I'm using variadic templates to fill the ..., the compiler somehow fails to see that the format string is really a literal.
> 
> I worked around this by introducing the printfVariableFormat trick, which allows the variadic template stuff to call something that doesn't warn.  I then hid the warning inside printfVariableFormat.

Would be nice to have a brief why comment saying that. I would write something like this:

    // Can't use WTF_ATTRIBUTE_PRINTF because clang does not understand the idiom where the templates build the variable argument list.
    // So instead we have to turn off this warning.

But what I really eventually would probably do is come up with something not based on the printf system at all. After all, we are only using this for a few simple bits of formatting, hex numbers and such.
Comment 17 Csaba Osztrogonác 2016-12-12 10:32:40 PST
EFL build is still broken because of this change:

../../Source/WTF/wtf/PrintStream.cpp: In member function ‘void WTF::PrintStream::printfVariableFormat(const char*, ...)’:
../../Source/WTF/wtf/PrintStream.cpp:55:28: error: function might be possible candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
     vprintf(format, argList);
                            ^
cc1plus: all warnings being treated as errors
Comment 18 Gavin Barraclough 2016-12-14 01:36:21 PST
Unfortunately had to rollout in r209795 due to memory use regression.
Comment 19 Filip Pizlo 2016-12-14 09:13:48 PST
(In reply to comment #18)
> Unfortunately had to rollout in r209795 due to memory use regression.

You rolled out the wrong patch.  This patch fixes a GC bug, so without it, the debug bots will go red.  Also, rolling this out won't fix the memory use regression.

I think you meant to roll out https://trac.webkit.org/changeset/209692.

A better option is to disable concurrent GC on the affected platform by editing Options.cpp.

Can you please roll this fix back in - it's needed regardless of whether concurrent GC is enabled.
Comment 20 Gavin Barraclough 2016-12-14 10:39:33 PST
Relanded fixed version in r209813.