Sorry, something went wrong.
There was a problem hiding this comment.
A couple notes (and one bug, I think).
Also, your stats seem to indicate that both the success and hit rates for LOAD_ATTR are lower with this change. Are we sure it's worth doing?
If so, this seems like quite a bit of extra code and work just to save a branch on the oparg's low bit at the end of the instruction, in my opinion. I would be surprised if the separate instructions were really more performant.
Sorry, something went wrong.
| if ((instr->op.arg & 1) == 0) { | ||
| if (specialize_attr_loadclassattr(owner, instr, name, descr, kind, false)) { | ||
| goto success; | ||
| } | ||
| } | ||
| else { | ||
| SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_CLASS_ATTR_SIMPLE); | ||
| } |
There was a problem hiding this comment.
Nit: I find this flow a bit easier to follow:
| if ((instr->op.arg & 1) == 0) { | |
| if (specialize_attr_loadclassattr(owner, instr, name, descr, kind, false)) { | |
| goto success; | |
| } | |
| } | |
| else { | |
| SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_CLASS_ATTR_SIMPLE); | |
| } | |
| if (instr->op.arg & 1) { | |
| SPECIALIZATION_FAIL(LOAD_ATTR, SPEC_FAIL_ATTR_CLASS_ATTR_SIMPLE); | |
| } | |
| else if (specialize_attr_loadclassattr(owner, instr, name, descr, kind, false)) { | |
| goto success; | |
| } |
Sorry, something went wrong.
There was a problem hiding this comment.
To me, it seems more natural to have the success first, then the failure. A matter of taste, I suppose.
Sorry, something went wrong.
|
The stats for LOAD_ATTR change from
to
which is a modest improvement, but the failure stats are now
Which means that fixing the materialization of __dict__ can get the specialization success (ignoring misses) rate up to about 97%, which is quite good. |
Sorry, something went wrong.
|
If so, this seems like quite a bit of extra code and work just to save a branch on the oparg's low bit at the end of the instruction. The oparg & 1 case is inserted for calls. It seems unlikely that a non-descriptor is being called much, as both Python functions and method-descriptors are descriptors. The stats support this. "class attr simple" is a tiny 0.1% of specialization failures with this change. |
Sorry, something went wrong.
|
The oparg & 1 case is inserted for calls. It seems unlikely that a non-descriptor is being called much, as both Python functions and method-descriptors are descriptors. The stats support this. "class attr simple" is a tiny 0.1% of specialization failures with this change. I'm confused. Wouldn't just adding a branch on (oparg & 1) at the end of the existing LOAD_ATTR_METHOD_WITH_VALUES and LOAD_ATTR_NONDESCRIPTOR_NO_DICT instructions have the exact same effect as this PR, but without adding two whole new instructions that are almost identical to the existing ones? |
Sorry, something went wrong.
|
I see what you mean now. I thought you were talking about the specialization, not the resulting instruction. I'll try that. |
Sorry, something went wrong.
|
The generated code is noticeably worse for the conditional case. Py_DECREF(self);
res = Py_NewRef(descr);
stack_pointer[-1] = res;
next_instr += 9;
but with the oparg test it looks like this: res2 = Py_NewRef(descr);
if (oparg & 1) {
res = self;
}
else {
Py_DECREF(self);
}
STACK_GROW(((oparg & 1) ? 1 : 0));
if (oparg & 1) { stack_pointer[-(((oparg & 1) ? 1 : 0))] = res; }
stack_pointer[-(1 + ((oparg & 1) ? 1 : 0))] = res2;
next_instr += 9;
So, I think the additional instruction is worth it. |
Sorry, something went wrong.
|
Note, the generated code actually has this sequence in it: STACK_GROW((0 ? 1 : 0));
if (0) { stack_pointer[-(1 + (0 ? 1 : 0))] = res2; }
but even the worst C compiler will eliminate that code. |
Sorry, something went wrong.
|
So, I think the additional instruction is worth it. Is the branchy form actually measurably slower? I would imagine that the C compiler would turn the five (oparg & 1) branches into just one or two, and that the resulting code would still be about the same (or better than) the cost of bulking out the interpreter. It's obviously not a huge deal (they're both okay approaches, which is why I approved this PR), but a branch at the end of the existing instruction feels easier to maintain than two new specialized forms. |
Sorry, something went wrong.
|
Is the branchy form actually measurably slower? Maybe not now, but it mostly likely will be when JIT compiled. Is the branchy form otherwise better? |
Sorry, something went wrong.
This PR specializes for things like obj.x where:
Stats
The miss rate for LOAD_ATTR_NONDESCRIPTOR_WITH_VALUES is quite poor at 33%
(Ignore the stats for LOAD_ATTR_NONDESCRIPTOR_LAZY_DICT, I've removed it)