I'll make a summary from what I understand... Maybe we should move this discussion to the VM list, but I'm not sure.
- Igor made changes to support the primitive 220 - those changes may try to activate a non-method object - I added a check to tell if it is a compiled method or not to the new code, so if it's not it flows through the old method handling (which uses the primitiveFunctionPointer I assume right from the CoInterpreter implementation) This, I see should be taken into account into #internalExecuteNewMethod and #executeNewMethod, which are preeeeeeety similar and both have the same change :). Maybe I'm making nonsense again, It's difficult to say to me. I just want to help fixing this. Tx!!! Guille On Fri, May 31, 2013 at 9:28 AM, Guillermo Polito <[email protected] > wrote: > Actually, from CoInterpreter: > > addNewMethodToCache: class > "Override to refuse to cache other than compiled methods. > This protects open PICs against having to test for compiled methods." > (objectMemory isOopCompiledMethod: newMethod) ifFalse: > [primitiveFunctionPointer := #primitiveInvokeObjectAsMethod. > ^self]. > super addNewMethodToCache: class > > > On Fri, May 31, 2013 at 9:20 AM, Guillermo Polito < > [email protected]> wrote: > >> Ahh, that's why the code in CoInterpreter I was looking didn't match the >> C code :D. Was looking at the wrong place, hehe. >> >> Igor, why do you say that this method should be reached only with vanilla >> compiled methods? From the comment I understand >> >> "For interpreter performance and to ease the objectAsMethod >> implementation eagerly >> evaluate the primtiive, i.e. if the method is cogged and has a >> primitive /do not/ evaluate >> the machine code primitive, just evaluate >> primitiveFunctionPointer directly." >> >> And in #addNewMethodToCache: there is as a first statement: >> >> [...] >> (objectMemory isOopCompiledMethod: newMethod) >> ifTrue: >> [primitiveIndex := self primitiveIndexOf: newMethod. >> primitiveFunctionPointer := self functionPointerFor: primitiveIndex >> inClass: class] >> ifFalse: >> [*primitiveFunctionPointer := #primitiveInvokeObjectAsMethod*]. >> [...] >> >> So I'd say that both objects as methods and primitives are kind of >> handled in the same way after? >> >> >> >> On Fri, May 31, 2013 at 2:27 AM, Igor Stasenko <[email protected]>wrote: >> >>> On 30 May 2013 20:06, Guillermo Polito <[email protected]> >>> wrote: >>> > Ok, I was playing with flushing caches and stuff... Its still a bit >>> obsure >>> > to me why the example in the workspace works and the code coverage >>> doesnot. >>> > >>> > However, I managed to hack the VM to make it work :). >>> > >>> > It looks like the VM is treating the objects as methods, as cog >>> methods. So >>> > I added the following validation in the commonSend: >>> > >>> > methodHeader2 = longAt((GIV(newMethod) + BaseHeaderSize) + >>> (HeaderIndex << >>> > ShiftForWord)); >>> > >>> > if (isCogMethodReference(methodHeader2) && >>> > isCompiledMethodHeader(methodHeader2)) { >>> > >>> > /* begin externalizeIPandSP */ >>> > >>> > assert((((usqInt)localIP)) != (ceReturnToInterpreterPC())); >>> > >>> > GIV(instructionPointer) = oopForPointer(localIP); >>> > >>> > GIV(stackPointer) = localSP; >>> > >>> > GIV(framePointer) = localFP; >>> > >>> > executeCoggedNewMethodmethodHeader(1, methodHeader2); >>> > >>> > } >>> > >>> > >>> > And it works :). >>> > >>> > I cc Eliot, so maybe he has an idea... >>> > >>> I touched this code. >>> >>> Original: >>> >>> CoInterpreter>>internalExecuteNewMethod >>> <inline: true> >>> "For interpreter performance and to ease the objectAsMethod >>> implementation eagerly >>> evaluate the primtiive, i.e. if the method is cogged and has a >>> primitive /do not/ evaluate >>> the machine code primitive, just evaluate >>> primitiveFunctionPointer directly." >>> primitiveFunctionPointer ~= 0 ifTrue: >>> [| succeeded | >>> self isPrimitiveFunctionPointerAnIndex ifTrue: >>> [^self internalQuickPrimitiveResponse]. >>> "slowPrimitiveResponse may of course context-switch. >>> If so we must >>> reenter the >>> new process appropriately, returning only if we've >>> found an >>> interpreter frame." >>> self externalizeIPandSP. >>> succeeded := self slowPrimitiveResponse. >>> instructionPointer = cogit ceReturnToInterpreterPC >>> ifTrue: >>> [instructionPointer := self iframeSavedIP: >>> framePointer]. >>> self internalizeIPandSP. >>> succeeded ifTrue: >>> [self return: self popStack toExecutive: true. >>> self browserPluginReturnIfNeeded. >>> ^nil]]. >>> "if not primitive, or primitive failed, activate the method" >>> (self methodHasCogMethod: newMethod) >>> ifTrue: [self iframeSavedIP: localFP put: localIP >>> asInteger. >>> instructionPointer := cogit >>> ceReturnToInterpreterPC. >>> self externalizeFPandSP. >>> self activateCoggedNewMethod: true. >>> self internalizeIPandSP] >>> ifFalse: [self internalActivateNewMethod] >>> >>> Now in subclass (NBCoInterpreter) >>> i made this: >>> >>> internalExecuteNewMethod >>> <inline: true> >>> "For interpreter performance and to ease the objectAsMethod >>> implementation eagerly >>> evaluate the primtiive, i.e. if the method is cogged and has a >>> primitive /do not/ evaluate >>> the machine code primitive, just evaluate >>> primitiveFunctionPointer directly." >>> >>> | methodHeader | >>> >>> methodHeader := self rawHeaderOf: newMethod. >>> >>> (self isCogMethodReference: methodHeader) ifTrue: [ >>> self externalizeIPandSP. >>> self executeCoggedNewMethod: true methodHeader: >>> methodHeader. >>> "we never reach here" >>> ]. >>> >>> primitiveFunctionPointer ~= 0 ifTrue: >>> [| succeeded | >>> self isPrimitiveFunctionPointerAnIndex ifTrue: >>> [^self internalQuickPrimitiveResponse]. >>> "slowPrimitiveResponse may of course context-switch. >>> If so we must >>> reenter the >>> new process appropriately, returning only if we've >>> found an >>> interpreter frame." >>> self externalizeIPandSP. >>> succeeded := self slowPrimitiveResponse. >>> instructionPointer = cogit ceReturnToInterpreterPC >>> ifTrue: >>> [instructionPointer := self iframeSavedIP: >>> framePointer]. >>> self internalizeIPandSP. >>> succeeded ifTrue: >>> [self return: self popStack toExecutive: true. >>> self browserPluginReturnIfNeeded. >>> ^nil]]. >>> "if not primitive, or primitive failed, activate the method" >>> (self methodHasCogMethod: newMethod) >>> ifTrue: [self iframeSavedIP: localFP put: localIP >>> asInteger. >>> instructionPointer := cogit >>> ceReturnToInterpreterPC. >>> self externalizeFPandSP. >>> self activateCoggedNewMethod: true. >>> self internalizeIPandSP] >>> ifFalse: [self internalActivateNewMethod] >>> >>> === >>> The intent of change was: >>> - if method is cog method, execute it (the jited code takes care to >>> call primitive, if it there) >>> >>> The old code logic was: >>> - run primitive if it there >>> - and only then, if no prim/or prim failed, try to run a jited code >>> (but entry point is to the first bytecode of method, >>> not to its start) >>> >>> This (old) logic prevents from using primitive 220 properly, because >>> it is a "meta-primitive" - a marker >>> that says that given method should: >>> - always be jited >>> - actual primitive is taken by copying machine code from compiled >>> method trailer into CogMethod's primitive section. >>> Therefore executing prim 220 (a C function) ~~ invoking method's >>> primitive (a machine code generated by image). >>> >>> Still i cannot understand why code reaching this point, when a result >>> of lookup is not a compiled method, but arbitrary object, because >>> everything even in original code says: VM enters this method only if >>> it found 100% vanilla guaranteed CompiledMethod, and not something >>> which pretends to be it. >>> As i understand , the logic to handle object-as-method is in method >>> lookup.. but not in internalExecuteNewMethod, because VM can "execute" >>> the only thing it knows, and it is CompiledMethod. >>> >>> Your test says, that Cog does something extra (some magic, which i >>> failed to grasp) with object-as-method, >>> like generating special kind of CogMethod which sends #run:as:in: >>> message to it. >>> >>> I would like to hear Eliot's comments on this. >>> >>> >>> >>> -- >>> Best regards, >>> Igor Stasenko. >>> >>> >> >
