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

Do we really need pre_hook and object_hook in update_from_external_object? #29

Open
jamadden opened this issue Sep 21, 2017 · 16 comments
Open

Comments

@jamadden
Copy link
Member

This project does not actually use them, and they add substantial overhead even when they aren't used.

Are they used internally? If so, for what? Can we find another way to do that? I just spot checked some places and in the core internal code, everybody passes None.

@jzuech3
Copy link

jzuech3 commented Sep 21, 2017

I believe nti.contenttypes.presentation (all the lesson assets) use pre_hooks.

@jamadden
Copy link
Member Author

For what? Is it just to get around implementing an internalizer class?

@papachoco
Copy link
Contributor

papachoco commented Sep 21, 2017

In trying to support legacy content we need to switch mimetypes; in addition there are places ntiid are a list .. instead of single field.

@jamadden
Copy link
Member Author

What? Can't you just register the internalizer for the old mime type?

@papachoco
Copy link
Contributor

We will.. give u a few days to add those in

@papachoco
Copy link
Contributor

object_hook is very legacy (use in assessments processing) and when updating a course outline.

@jamadden
Copy link
Member Author

This is a major feature request, it won't go in without a new PR and it could easily be postponed, take all the time you need. And if a new ZCML directive (I'm assuming the non-legacy internalizers are registered via <registerAutoPackageIO>) would help, let me know that too.

@jamadden
Copy link
Member Author

object_hook is very legacy (use in assessments processing) and when updating a course outline.

Without putting too much time into it right now, any idea what it would take to get rid of that?

@papachoco
Copy link
Contributor

I'd say about a week .. counting on @jzuech3 time

@papachoco
Copy link
Contributor

I thin we can register mimetype factories for those legacy objects. I don't think we need a a new directive.

<adapter factory=".datastructures.***"
		 for="nti.base.interfaces.IDict"
		 provides="nti.externalization.interfaces.IMimeObjectFactory"
		 name="application/vnd.nextthought.****" />

@jamadden
Copy link
Member Author

OK. I would consider this on the nice-to-have list. It will directly improve the performance of all object reads proportional to how many objects there are by some factor. Function calls in CPython, even trivial ones, aren't super cheap.

There may be some other things we can do for the no-op versions of the functions to get a lot of the way there to removing them in terms of perf, I'd have to benchmark...but that wouldn't improve the code clarity.

@jamadden
Copy link
Member Author

I thin we can register mimetype factories for those legacy objects.

Yeah, the <adapter> ZCML is what you need. I was just thinking that one that lets you register like an existing adapter but with a new name (much like the security descriptors let you do for classes) might be nice. Just depends on how many there are:

<ext:registerMimeFactoryAlias 
    like="the new mime type"
    name="the old mime type" />

@papachoco
Copy link
Contributor

There are about 19 legacy obejcts .. we could use such new directive command for 9 of them.

@jamadden
Copy link
Member Author

Probably not worth it, then, especially since these should theoretically ultimately go away.

@papachoco
Copy link
Contributor

Cool.. We could always release nti.externalization 1.0 with deprecated object_hook pre_hook while we remove the code

@jamadden
Copy link
Member Author

Yup!

jamadden added a commit that referenced this issue Dec 4, 2017
Refs #29

Relevant benchmarks before (cpython 2.7):

nti.externalization.tests.benchmarks.bm_simple_iface: fromExternalObject: Mean +- std dev: 168 us +- 7 us
nti.externalization.tests.benchmarks.bm_simple_registered_class: fromExternalObject: Mean +- std dev: 38.8 us +- 16.3 us

After:

nti.externalization.tests.benchmarks.bm_simple_iface: fromExternalObject: Mean +- std dev: 164 us +- 10 us
nti.externalization.tests.benchmarks.bm_simple_registered_class: fromExternalObject: Mean +- std dev: 32.8 us +- 1.7 us
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants