Bug 165741 – MarkedBlock::marksConveyLivenessDuringMarking should take into account collection scope

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 296857 [details]
the patch

[WebKit Commit Bot]

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.

[Filip Pizlo]

Created attachment 296876 [details]
the patch

[WebKit Commit Bot]

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.

[Saam Barati]

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?

[Filip Pizlo]

*** Bug 165735 has been marked as a duplicate of this bug. ***

[Saam Barati]

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.

[Filip Pizlo]

(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.

[Saam Barati]

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.

[Saam Barati]

Comment on attachment 296876 [details]
the patch

r=me

If others have concerns about the new logging code, they can be addressed later.

[Filip Pizlo]

CC'd some folks since this adds more to WTF.

[Filip Pizlo]

I'm going to wait for EWS.

[Darin Adler]

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

[Filip Pizlo]

(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!

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/209691

I'll land follow-up fixes soon.

[Darin Adler]

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.

[Csaba Osztrogonác]

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

[Gavin Barraclough]

Unfortunately had to rollout in r209795 due to memory use regression.

[Filip Pizlo]

(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.

[Gavin Barraclough]

Relanded fixed version in r209813.