RESOLVED FIXED 165741
MarkedBlock::marksConveyLivenessDuringMarking should take into account collection scope
https://bugs.webkit.org/show_bug.cgi?id=165741
Summary MarkedBlock::marksConveyLivenessDuringMarking should take into account collec...
Filip Pizlo
Reported 2016-12-10 23:09:26 PST
Patch forthcoming.
Attachments
the patch (56.08 KB, patch)
2016-12-10 23:35 PST, Filip Pizlo
no flags
the patch (56.23 KB, patch)
2016-12-11 16:26 PST, Filip Pizlo
saam: review+
Filip Pizlo
Comment 1 2016-12-10 23:35:32 PST
Created attachment 296857 [details] the patch
WebKit Commit Bot
Comment 2 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.
Filip Pizlo
Comment 3 2016-12-11 16:26:55 PST
Created attachment 296876 [details] the patch
WebKit Commit Bot
Comment 4 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.
Saam Barati
Comment 5 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?
Filip Pizlo
Comment 6 2016-12-11 16:46:02 PST
*** Bug 165735 has been marked as a duplicate of this bug. ***
Saam Barati
Comment 7 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.
Filip Pizlo
Comment 8 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.
Saam Barati
Comment 9 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.
Saam Barati
Comment 10 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.
Filip Pizlo
Comment 11 2016-12-11 16:54:58 PST
CC'd some folks since this adds more to WTF.
Filip Pizlo
Comment 12 2016-12-11 16:58:39 PST
I'm going to wait for EWS.
Darin Adler
Comment 13 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
Filip Pizlo
Comment 14 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!
Filip Pizlo
Comment 15 2016-12-11 17:30:29 PST
Landed in https://trac.webkit.org/changeset/209691 I'll land follow-up fixes soon.
Darin Adler
Comment 16 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.
Csaba Osztrogonác
Comment 17 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
Gavin Barraclough
Comment 18 2016-12-14 01:36:21 PST
Unfortunately had to rollout in r209795 due to memory use regression.
Filip Pizlo
Comment 19 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.
Gavin Barraclough
Comment 20 2016-12-14 10:39:33 PST
Relanded fixed version in r209813.
Note You need to log in before you can comment on or make changes to this bug.