Skip to content
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

[WIP] [#1078] package_show performance improvements #1079

Closed
wants to merge 8 commits into from

Conversation

wardi
Copy link
Contributor

@wardi wardi commented Jul 2, 2013

Comments requested. These changes aren't perfect, lots of tests are failing, but I'm sure they can be fixed with a little help. I would like feedback on the changes below before proceeding.

Performance numbers so far, taking best of 10 runs for each:

option time(s) improvement
original 0.074 1.0x
53a4e5c 0.038 1.9x
973bb8c 0.015 4.9x
f2a4822 0.009 8.2x

@tobes
Copy link
Contributor

tobes commented Jul 3, 2013

@wardi We mark work in progress pull requests like this as [WIP] in the title. I've added this.

Also we try to link our pull requests to the issue they are connected to. I use a crappy script I wrote https://github.com/tobes/gitlink other people use https://github.com/defunkt/hub or other solutions for this. This is not super important if you are just doing the odd issue/pull request, but it is really helpful as it reduces the number of issues and keeps the discussions in one place. Don't worry too much but it'd be good if you can do this. Also it might be harder with external repos but should be fine.

@tobes
Copy link
Contributor

tobes commented Jul 3, 2013

fixes issue #1078

@wardi
Copy link
Contributor Author

wardi commented Jul 3, 2013

@tobes does that command let me associate a PR with the original issue, instead of creating a separate issue? That would be nice. I assume it's too late to do that for this one now.

@domoritz
Copy link
Contributor

domoritz commented Jul 3, 2013

@wardi Yes, gitlink does it automatically for the current repo and branch you are in. However, your branch has to have the issue id in the name.

If you use hub, the command is something like hub pull-request -i 1079 -b okfn:master -h wardi:package_show-performance.

package_dict, errors = _validate(package_dict, schema, context=context)

for item in plugins.PluginImplementations(plugins.IPackageController):
item.after_show(context, package_dict)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a problem. If I add/remove a plugin from my .ini then it should change the results I see.

I'm not sure how we deal with this currently with the solr index.

unfortunately this part may need the json -> dict -> json cycle if IPackageControllers exist I'm not sure how we can get around this with the current interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if our schemas were possible to serialize (using converter names instead of functions, say) then the validated data could store the hash of the schema that was used for generating it

@wardi
Copy link
Contributor Author

wardi commented Jul 9, 2013

all parts of this PR are now present, with bug fixes, in #1088, #1104 and #1105

@wardi wardi closed this Jul 9, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants