Patch forthcoming.
Created attachment 296857 [details] the patch
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.
Created attachment 296876 [details] the patch
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 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?
*** Bug 165735 has been marked as a duplicate of this bug. ***
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.
(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 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 on attachment 296876 [details] the patch r=me If others have concerns about the new logging code, they can be addressed later.
CC'd some folks since this adds more to WTF.
I'm going to wait for EWS.
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
(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!
Landed in https://trac.webkit.org/changeset/209691 I'll land follow-up fixes soon.
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.
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
Unfortunately had to rollout in r209795 due to memory use regression.
(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.
Relanded fixed version in r209813.