Currently PIC caching is actually monomorphic.
Created attachment 25303 [details] The patch
Comment on attachment 25303 [details] The patch Great change! I have some nit-pick-type style comments. Generally the use of "proto" everywhere to mean "prototype" is a little confusing to me, because the prefix "proto" already exists and has a different meaning. I'd prefer that we actually write out prototype unless it makes things unbearably wordy and long. > + for (int i=0; i < count; ++i) { Tiny formatting thing: We normally put spaces around the "=" sign. > +#define ProtoStructureList_LENGTH 4 Does this need to be a macro rather than a const size_t? Similarly, do we need to use the unconventional capitalization here? > + // Only for the jitterbug, for nows. > + ASSERT_NOT_REACHED(); LOL. I don't really know what the comment means, but it's still funny. > + if (baseValue->isObject() && > + slot.isCacheable() && > + !asCell(baseValue)->structure()->isDictionary() && > + slot.slotBase() == asCell(baseValue)->structure()->prototypeForLookup(callFrame)) { We normally try to format these with the operators at the starts of lines instead of the ends -- I think it makes things read a little bit clearer. Those two places where you call asCell, I think I'd prefer asObject, because you've already established it's an object, and it's possible that we might some day implement an operation like structure() slightly more efficiently for JSObject than a JSCell. OK, it's not a realistic possibility, but still I like types to be as specific as possible in case that helps with optimization later. > + JSCell* baseCell = asCell(baseValue); Same thought here. I also think that if the values are worth putting into local variables for the code, maybe it's worth using a nested if and using the local variables in the if conditional too. > + // Heavy access to a prototype is a good indication that it's not being > + // used as a dictionary. Great comment, but I don't understand how the single call here indicates "heavy use". You might want to word it slightly differently to make that clearer. > +JSValue* Interpreter::cti_op_get_by_id_proto_list_append(CTI_ARGS) It's a bit disappointing that so much code is repeated. If there's a way to factor things out into separate inline functions, that would be nice. Obviously ARG_xxx causes trouble for that sort of thing, and I wouldn't want to sacrifice performance, but it's worth considering. > + CallFrame* callFrame = ARG_callFrame; > + Identifier& ident = *ARG_id2; > + > + JSValue* baseValue = ARG_src1; > + PropertySlot slot(baseValue); > + JSValue* result = baseValue->get(callFrame, ident, slot); I personally write these functions using fewer local variables. I would use ARG_callFrame and *ARG_id2 directly rather than putting them into locals. I'm not sure why I prefer that style -- I guess I just think that if no clarity is sacrificed I'd prefer using fewer lines of code and I even think that our ARG trick might work better if we reduce the chance of confusing the compiler into allocating another stack location to store the ARG thing again as a local variable. It's a minor point, and my habit is not necessarily a good thing. Obviously naming the things is a form of documentation. > + // check eax is an object of the right Structure. Better to capitalize the first word of a sentence comment like this one. > + __ cmpl_i32m(reinterpret_cast<uint32_t>(structure), FIELD_OFFSET(JSCell, m_structure), X86::eax); In general, I look for ways to use inline casting functions or function templates rather than actually sprinkling the code with these reinterpret_cast and other typecasts. Somehow I think this could make some future refactorings easier. I think we should make one for casting function pointers to void*, for example, and use that instead of using reinterpret_cast directly. And the one for casting pointers to integers. If the functions are suitable task-specific, then I think they could help improve code readability. > + __ cmpl_i32m(reinterpret_cast<uint32_t>(prototypeStructure), static_cast<void*>(prototypeStructureAddress)); Is the static_cast<void*> needed? Why? > + intptr_t successDest = (intptr_t)(stubInfo->hotPathBegin) + repatchOffsetGetByIdPropertyMapOffset; Lets use C++ style casts, not C-style, whenever possible. > + X86Assembler::link(code, success, reinterpret_cast<void*>(successDest)); We could use char* instead of intptr_t when we need to do pointer arithmetic. That would eliminate the need for this reinterpret_cast back to void*. > + // Track the stub we have created so that it will be deleted later. > + //info.stubRoutine = code; Commented out line of code??? What's up with this? > + // FIXME: should revert this repatching, on failure. Is this something important to get back to? (Also, please capitalize first word of the sentence comment.) r=me -- the only thing that worries me is that line of commented-out code
// Uncacheable: give up. - if (!slot.isCacheable()) { + if (!baseValue->isObject() || !slot.isCacheable()) { ctiRepatchCallByReturnAddress(returnAddress, reinterpret_cast<void*>(cti_op_get_by_id_generic)); return; Was this the bug fix you mentioned on IRC? If so, let's land it separately. (I'd rather fix the bug in caching primitive properties, instead of disabling caching for primitive properties.)
(In reply to comment #2) > (From update of attachment 25303 [details] [review]) > > > + // Only for the jitterbug, for nows. > > + ASSERT_NOT_REACHED(); > > LOL. I don't really know what the comment means, but it's still funny. Ooops, :-) this comment should have been upgraded to something more grown-up for checkin, done now. > Those two places where you call asCell, I think I'd prefer asObject > > + JSCell* baseCell = asCell(baseValue); > Same thought here. > > I also think that if the values are worth putting into local variables for the > code, maybe it's worth using a nested if and using the local variables in the > if conditional too. Agreed, but I won't make this change just now, ... this gets tangled up into a separate existing bug (segfault in ToT) that I'm about to raise - we should be able to cache property accesses for non-object non-immediate primitives, so the asCell checks are probably correct, and the isObject one is not. > > + // Heavy access to a prototype is a good indication that it's not being > > + // used as a dictionary. > > Great comment, but I don't understand how the single call here indicates "heavy > use". You might want to word it slightly differently to make that clearer. Agreed, it's a good comment - sadly I just copied it along with this chunk of code. :-( Will forward these comments along to the appropriate authorities (Geoff, I believe). > > +JSValue* Interpreter::cti_op_get_by_id_proto_list_append(CTI_ARGS) > > It's a bit disappointing that so much code is repeated. Fixed! - I've merged these two functions, deleting a whole pile of code. > > + // Track the stub we have created so that it will be deleted later. > > + //info.stubRoutine = code; > > Commented out line of code??? What's up with this? Ah, my bad, the commented out code is not needed. This trampoline is based on the monomorphic prototype trampoline, which is tracked in the stub info object, whereas these new stubs are tracked on the new prototype structure list object. > > + // FIXME: should revert this repatching, on failure. > Is this something important to get back to? I've deleted these comments - this shouldn't really be a fixme, the right thing to do here isn't that obvious. In the case we have cached a property access that starts missing we might want to un-cache it (since it will introduce a penalty in the missing cases, which may be more common). This may not, however be the right thing to do
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 25303 [details] [review] [review]) Oh, forgot to say, all other review comments should be fixed too. And ggaren – yes, and okay, reverted.
Sending JavaScriptCore/ChangeLog Sending JavaScriptCore/bytecode/CodeBlock.cpp Sending JavaScriptCore/bytecode/Instruction.h Sending JavaScriptCore/bytecode/Opcode.h Sending JavaScriptCore/interpreter/Interpreter.cpp Sending JavaScriptCore/interpreter/Interpreter.h Sending JavaScriptCore/jit/JIT.cpp Sending JavaScriptCore/jit/JIT.h Transmitting file data ........ Committed revision 38652.