order layers by MRO before doing topological sort #14
Conversation
davisagli
force-pushed
the
davisagli-layer-ordering
branch
from
1ca8518
to
9692646
Compare
| def layer_sortkey(layer): | ||
| return tuple((c.__module__, c.__name__) for c in getmro(layer)[::-1] if c is not object) | ||
|
|
||
| layers = [layer for layer in layers] |
There was a problem hiding this comment.
I think this would be better spelled layers = list(layers).
There was a problem hiding this comment.
I tried that first, but the function is called with a dict in one case (in which case we want the keys) and a list in other cases.
There was a problem hiding this comment.
list() on a dict gives you a list of the dict's keys.
|
A 30% test speedup sounds like a worthwhile goal! I'm worried about merging this without tests for the new behavior. Now I know zope.testrunner's tests are very painful (it's why I stopped contributing to zope.testrunner myself), but would you please try? |
|
Sure, I'll try to add a test in the next day or two. |
|
I noticed in one of your previous rebases that you had a side effect that made the unit test layer run first. I got very excited about that (see bug #497871 in the old tracker) before noticing the final diff. Oh well, perhaps we can reuse the test changes from c8282f2 in a separate PR. |
|
I was excited about that too. Might take another look. And looks like that bug has some unit tests which I can adapt. |
do3cc
force-pushed
the
davisagli-layer-ordering
branch
from
19701c3
to
3e5498a
Compare
|
I've created a different PR with unittests first. basically by modifying this PR |
Currently the test runner sorts layers by name and then does a topological sort to make sure that a layer runs after the layers it depends on. This is suboptimal if there are many layers that share ancestry; it doesn't do anything to make sure those layers run consecutively, so we end up setting up and tearing down layers more than necessary.
This changes order_by_bases to do an initial sort by reversed MRO instead of by name so that layers with shared ancestry end up adjacent. This appears to speed up Plone's tests by about 30%.
I haven't run the zope.testrunner tests yet with this change, so let's see what travis says...