Bug 25832 – Refactor JIT code-handle objects.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

The representation of generated code is currently a bit ofa mess.  We have a class JITCode which wraps the pointer to a block of generated code, but this object does not reference the executable pool meaning that external events (the pool being derefed) could make the pointer become invalid.  To overcome this both the JIT and Yarr implement further (and similar) objects to wrap teh code pointer with a RefPtr to the pool.  To add to the mire, as well as the CodeBlock containing a handle onto the code the FunctionBodyNode also contains a copy of the code pointer which is used almost (but not entirely) uniquely to access the JIT code for a function.

[Gavin Barraclough]

Created attachment 30409 [details]
The patch

[Gavin Barraclough]

Created attachment 30410 [details]
Fix bdash's review comments.

[Darin Adler]

Comment on attachment 30410 [details]
Fix bdash's review comments.

> +        wrap teh code pointer with a RefPtr to the pool.  To add to the mire, as well as the

"teh"

> +            AssemblerType::patchMacroAssemblerCall(reinterpret_cast<intptr_t>(this->m_location), reinterpret_cast<void*>(label.getJumpDestination()));

Why the "this->" here?

> +        CodeRef()
> +            : m_code(0)
> +        {
> +        }

Should this initialize size to 0 in debug versions?

> +        void* m_code;
> +        RefPtr<ExecutablePool> m_executablePool;
> +#ifndef NDEBUG
> +        size_t m_size;
> +#endif

In the past we've normally made struct members have no prefix, and used the m_ prefix for private class members. Any reason to not just make this a class? Or you could remove the m_ prefixes? Or just leave as is if you like.

Would it be OK to copy a CodeRef? If not, then I suggest inheriting from Noncopyable.

> +        template <class AssemblerType_T>

The _T in the name is not something we do anywhere else. I think you should just omit it.

I suggest making PatchBuffer inherit from Noncopyable too. Why doesn't PatchBuffer just have a private CodeRef data member?

> +        inline void complete()
> +        {
> +#ifndef NDEBUG
> +            m_completed = true;
> +#endif
> +        }

No need for the "inline" here. Also, this doesn't need to go before the uses of it. Within a class definition, all the compilers handle things in any order.

I think you should put the ASSERT(!m_completed) inside the complete() function. I think the complete() function needs a different name such as markAsComplete(). The ambiguity about complete which could be either a verb or adjective makes it not such a good name.

> +        CodeRef finalize()
> +        {
> +            ASSERT(!m_completed);
> +            complete();
> +
> +            return CodeRef(m_code, m_executablePool, m_size);
> +        }
> +        CodeLocationLabel entry()
> +        {
> +            ASSERT(!m_completed);
> +            complete();
> +
> +            return CodeLocationLabel(m_code);
> +        }

It's a bit weak to name the function entry() when it's an action with a side effect. Also, I don't think you need to explicitly call the constructor CodeLocationLabel -- implicit conversion will take care of it.

> +#if ENABLE(JIT)
> +#endif

You should delete this.

> +        void setJITCode(JITCode jitCode);

No need for the argument name jitCode here.

> -        static void linkCall(JSFunction* callee, CodeBlock* calleeCodeBlock, JITCode ctiCode, CallLinkInfo* callLinkInfo, int callerArgCount);
> +        static void linkCall(JSFunction* callee, CodeBlock* calleeCodeBlock, JITCode&, CallLinkInfo* callLinkInfo, int callerArgCount);

No need for the argument name callLinkInfo here. And I do hate "info".

> +#include "MacroAssembler.h"
>  #include "CallFrame.h"
>  #include "JSValue.h"
>  #include "Profiler.h"

Please keep include sorted alphabetically.

> +        JITCode(MacroAssembler::CodeRef ref)
> +            : m_ref(ref)
>          {
>          }

Maybe the argument should be const MacroAssembler::CodeRef&? You might want to do this:

    typedef MacroAssembler::CodeRef CodeRef;

Inside this class you could them omit the CodeRef thing.

>          operator bool() const
>          {
> -            return code != 0;
> +            return m_ref.m_code != 0;
>          }

You don't need the != 0 here -- bool does that automatically.

It's not good style to have operator bool. You should either use an explicitly named function if you want to be clean, or use the safe operator bool idiom from RefPtr.

> +#ifndef NDEBUG
> +        inline size_t size()
> +        {
> +            ASSERT(m_ref.m_code);
> +            return m_ref.m_size;
> +        }
> +#endif

It's pointless to have this marked inline for two reasons. First, all functions declared inside class definitions are always inline. Second, when NDEBUG is not defined, we don't do any inlining anyway. I don't think there's any good reason to put this function inside an #if -- what's wrong with asking the size outside debugging code?

I'm not really happy about the use of void* for code pointers. Lack of any type safety means it's unnecessarily easy to pass the wrong argument. I think we should come up with a scheme for giving executable code a type.

> +        ExecutablePool* getExecutablePool()
> +        {
> +            return m_jitCode.executablePool();
> +        }

I see a reason to use "get" in the function name here.

> +        void setJITCode(JITCode jitCode)
> +        {
> +            m_jitCode = jitCode;
> +        }

Should the argument be const JITCode&?


> -    if (jitObject.m_pcreFallback) {
> -        int result = jsRegExpExecute(jitObject.m_pcreFallback, input, length, start, output, outputArraySize);
> +    if (jitObject.pcreFallback()) {
> +        int result = jsRegExpExecute(jitObject.pcreFallback(), input, length, start, output, outputArraySize);
>          return (result < 0) ? -1 : output[0];
> -    } else {
> -#if PLATFORM(X86) && !COMPILER(MSVC)
> -        typedef int (*RegexJITCode)(const UChar* input, unsigned start, unsigned length, int* output) __attribute__ ((regparm (3)));
> -#else
> -        typedef int (*RegexJITCode)(const UChar* input, unsigned start, unsigned length, int* output);
> -#endif
> -        return reinterpret_cast<RegexJITCode>(jitObject.m_jitCode)(input, start, length, output);
> -    }
> +    } else
> +        return jitObject.execute(input, start, length, output);

We normally don't do else after return.

>  
> +
> +#if PLATFORM(X86) && !COMPILER(MSVC)
> +#define YARR_CALL __attribute__ ((regparm (3)))
> +#else
> +#define YARR_CALL
> +#endif
> +
> +

We normally use single blank lines, never double.

> +    JSRegExp* pcreFallback() { return m_pcreFallback; }
> +    void setFallback(JSRegExp* fallback) { m_pcreFallback = fallback; }

Can this just be named fallback? Why "pcreFallback" in the getter but not the setter?

> +    operator bool() { return m_ref.m_code; }

Same thought as above about operator bool. It's not safe!

I had enough nitpicks that I'll say review-.

[Gavin Barraclough]

(skipping over straightforward fixes).

> > +            AssemblerType::patchMacroAssemblerCall(reinterpret_cast<intptr_t>(this->m_location), reinterpret_cast<void*>(label.getJumpDestination()));
> 
> Why the "this->" here?

I wrote this code, and I have no idea why this is necessary.  The variable m_location is contained in a parent class and is being accessed from a derived type, but that is no reason for the explicit 'this' dereference to be required.  Effectively the code here can be reduced down to this:

    class A { protected: int v; };
    class B : public A { public: int getV() { return v; } };

I see no reason why it should be necessary to write this->v  in getV() here – and in this reduced example it is not necessary.  But in the MacroAsembler label types, removing the this-> causes it to break (cease to compile).  I'm not sure what is different.  My best guess would be that this could be related to these classes being nested inside a templated outer class, though I don't really see why this should cause a scoping issue.

Can you see what I'm missing here?

> Would it be OK to copy a CodeRef? If not, then I suggest inheriting from
> Noncopyable.
>
> [[from later in the review]]
>
> I'm not really happy about the use of void* for code pointers. Lack of any type
> safety means it's unnecessarily easy to pass the wrong argument. I think we
> should come up with a scheme for giving executable code a type.

I quite agree – and this patch is intended to be another step towards improving this problem!

In most situations, it is also unsafe to retain a pointer to generated code without also retaining a ref-count on the executable pool containing the code.  As such, I'd hope that for a large number of situation where one might pass a void* to generated code, we can now instead use a CodeRef (or a JITCode or RegexCodeBlock object, which carry more semantic information as to how this block of code can be used).  As such, I'd say that it is important that the CodeRef is copyable – it would be a reasonable requirement to have to references to the same piece of code, and we don't want people to have to unwrap the void* then wrap it back up into another CodeRef just to achieve this.

As I'm sure you're aware we've been working to remove uses of void*, with many already replaced with MacroAssembler::CodeLocation* and MacroAssembler::ProcessorReturnAddress types.  There is clearly still more work to do here.  The next one to fix are likely the void*s on JITStubs object pointing to a set of shared trampolines; these should probably be replaced by an appropriate MacroAssembler::Codelocation* type (which will possibly just be a existing CodeLocationLabel, though this is a bit of a catch-all, might be worth a little thinking into whether we can do something more useful here.)

> I think the complete() function needs a different name such as
> markAsComplete(). The ambiguity about complete which could be either a verb or
> adjective makes it not such a good name.
> > +        CodeRef finalize()
> > +        {
> > +        }
> > +        CodeLocationLabel entry()
> > +        {
> > +        }
> 
> It's a bit weak to name the function entry() when it's an action with a side
> effect. Also, I don't think you need to explicitly call the constructor
> CodeLocationLabel -- implicit conversion will take care of it.

Okay, I've renamed, these functions - the public functions to obtain the final code:

    finalizeCode() and finalizeCodeAddendum()

Which were formerly finalize() and entry() respectively (the latter being used to generate trampolines attached to existing blocks of code).
And I've renamed the complete() method, called by the two methods above, as:

    performFinalization()

I'm not keen on markAsComplete because to me it implies that the purpose of this method is simply to track whether completion has occurred.  This is not my intention - my intention is for this method to be available to perform any tasks necessary at the point code generation is complete (albeit that this set of tasks is currently null :-) ).

Thoughts/comments on the new names?

> > +#ifndef NDEBUG
> > +        inline size_t size()
> > +        {
> > +            ASSERT(m_ref.m_code);
> > +            return m_ref.m_size;
> > +        }
> > +#endif
> 
> It's pointless to have this marked inline for two reasons. First, all functions
> declared inside class definitions are always inline. Second, when NDEBUG is not
> defined, we don't do any inlining anyway. I don't think there's any good reason
> to put this function inside an #if -- what's wrong with asking the size outside
> debugging code?

'inline' removed.  This code is !NDEBUG only because the CodeRef only retains the size of the code in !NDEBUG builds, to reduce memory overhead.

All other comments are hopefully fixed in the new version of the patch.
G.

[Gavin Barraclough]

Created attachment 30426 [details]
Fixes for darin's review comments.

[Gavin Barraclough]

Created attachment 30427 [details]
Gah, renamed methods should be renamed in the ChangeLog, too.

[Gavin Barraclough]

Created attachment 30428 [details]
Double Gah, removing accidentally committed example code written for review comments.

[Darin Adler]

Comment on attachment 30428 [details]
Double Gah, removing accidentally committed example code written for review comments.

> -        template<class AssemblerType_T>
> +        template<class g>
>          friend class AbstractMacroAssembler;

I don't think you meant to say "g" here.

> +        void relinkCallerToTrampoline(CodeLocationLabel label)
> +        {
> +            AssemblerType::patchMacroAssemblerCall(reinterpret_cast<intptr_t>(this->m_location), reinterpret_cast<void*>(label.getJumpDestination()));
> +        }

This seems to be casting void* to void* using reinterpret_cast. Lets leave that cast out.

> +#ifndef NDEBUG
> +            , m_size (masm->m_assembler.size())
> +#endif

Stray space here after m_size.

r=me

[Gavin Barraclough]

Sending        JavaScriptCore/ChangeLog
Sending        JavaScriptCore/assembler/AbstractMacroAssembler.h
Sending        JavaScriptCore/bytecode/CodeBlock.cpp
Sending        JavaScriptCore/bytecode/CodeBlock.h
Sending        JavaScriptCore/interpreter/CallFrameClosure.h
Sending        JavaScriptCore/interpreter/Interpreter.cpp
Sending        JavaScriptCore/jit/JIT.cpp
Sending        JavaScriptCore/jit/JIT.h
Sending        JavaScriptCore/jit/JITCode.h
Sending        JavaScriptCore/jit/JITPropertyAccess.cpp
Sending        JavaScriptCore/jit/JITStubs.cpp
Sending        JavaScriptCore/parser/Nodes.cpp
Sending        JavaScriptCore/parser/Nodes.h
Sending        JavaScriptCore/runtime/RegExp.cpp
Sending        JavaScriptCore/yarr/RegexJIT.cpp
Sending        JavaScriptCore/yarr/RegexJIT.h
Transmitting file data ................
Committed revision 43837.

[Gavin Barraclough]

Reverted, resubmitted in 44030.