On Apr 19, 2014, at 13:36, Filip Pizlo <fpi...@apple.com> wrote:
> Here are some examples.
>
> Guarding a loop that does assertions:
>
> if (!ASSERT_DISABLED) {
> for (unsigned index = 0; index < m_generationInfo.size(); ++index) {
> GenerationInfo& info = m_generationInfo[index];
> RELEASE_ASSERT(!info.alive());
> }
> }
>
> The one above comes from DFGSpeculativeJIT.cpp, and it is in a non-trivial
> method. Using an #if here would obfuscate the control flow and make the
> method harder to maintain. Arguably, a ASSERT would have been fine because
> the compiler "should" be smart enough to kill the loop if the ASSERT is a
> no-op. I'm not sure all compilers would be smart enough, though -
> particularly if m_generationInfo is any kind of interesting collection class.
How does using an #if obfuscate the control follow and make the method harder
to maintain?
> Guarding code in the JIT that emits an assertion:
>
> if (!ASSERT_DISABLED) {
> JITCompiler::Jump ok = m_jit.branch32(
> JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));
> m_jit.breakpoint();
> ok.link(&m_jit);
> }
>
> The one above also comes from the DFG JIT. We have lots of code like this.
> The assertions are very useful but obviously we don't want them in a release
> build. Using an #if would make the code harder to maintain.
How would using an #if make the code harder to maintain?
> Counterexample where we using an #if to guard an assertion in the JIT:
>
> #if !ASSERT_DISABLED
> SpeculateCellOperand op1(this, node->child1());
> JITCompiler::Jump isOK = m_jit.branchStructurePtr(
> JITCompiler::Equal,
> JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()),
> node->structure());
> m_jit.breakpoint();
> isOK.link(&m_jit);
> #else
> speculateCell(node->child1());
> #endif
>
> Code like this is harder to understand, especially if the code being guarded
> is longer than a page. I don't think we should encourage this style, when it
> would have been clearer to do:
>
> if (!ASSERT_DISABLED) {
> SpeculateCellOperand op1(this, node->child1());
> JITCompiler::Jump isOK = m_jit.branchStructurePtr(
> JITCompiler::Equal,
> JITCompiler::Address(op1.gpr(), JSCell::structureIDOffset()),
> node->structure());
> m_jit.breakpoint();
> isOK.link(&m_jit);
> } else
> speculateCell(node->child1());
I disagree with your assertion that the latter is clearer. It’s certainly
different, but how is it an improvement?
- Mark
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
