-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Speed up XML loading #121
Speed up XML loading #121
Conversation
Without this we see objects not having a `parent` attribute at all.
350031e
to
2abb5b5
Compare
I think given that this is a smaller patch I'm happy to merge this ahead of #117 and look at the fallout there. I am slightly concerned about the stripProxy possibly having a performance problem as I have highlighted. |
Just out of curiosity @milliams have you run any integration tests on this branch? |
If we merge #122 then I will merge those changes into this branch and see how they do. I haven't given it a completely thorough test yet, no. I will certainly not be merging until the tests have completed. |
All the currently converted integration tests work correctly as well as all the unit tests. |
I'm happy to merge this in that case :) |
Ok, great. I will go ahead and merge shortly. I suggest merging develop into your branches and pushing the changes to GitHub so that the integration tests can run. |
I'm playing with that now, there's at least one thing has cropped up so I will likely need some time to get #117 into a sensible state |
I spent some time getting the XML loading working faster. On my benchmark of:
I was finding load times of 7 seconds on latest develop. After these changes I'm now seeing it between 4 and 4.5 seconds.
The bulk of the changes in here are removing redundant checks in Objects.py for attributes which always exist (or at least do after making the objects be created correctly). It also changes some
isType
forisinstance
(that alone gives almost a second speed-up).Most importantly is that his PR adds 52 lines of code but removes 154.
I've run my local tests over this and have seen no regressions but I'd welcome a second and third pair of eyes.
It's possible that this clashes with #117 and #119 but since this patch is much smaller it shouldn't be a problem.