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

Add additional hooks for subclasses #24

Merged
merged 3 commits into from
Sep 6, 2019
Merged

Add additional hooks for subclasses #24

merged 3 commits into from
Sep 6, 2019

Conversation

jamadden
Copy link
Member

@jamadden jamadden commented Sep 5, 2019

Fixes #22

I see nti.site doing something like this in its subclass:

class _RunJobInSite(TransactionLoop):

    def setUp(self):
        db = component.getUtility(IDatabase)
        self.conn = db.open()
        self.tearDown = self.conn.close

That means the object can't be used from multiple threads at the same time, but it's not used in that way anyway.

Also some minor refactoring with an eye towards speed and simplicity.

We get between 5 and 10% faster, even accounting for all the overhead of raising exceptions:

Benchmark master this branch
trivial commit 14.9 us 13.2 us: 1.12x faster (-11%)
max retries 16.0 us 15.1 us: 1.06x faster (-5%)

Fixes #22

Also some minor refactoring with an eye towards speed and simplicity.
We get between 5 and 10% faster, even accounting for all the overhead
of raising exceptions:

+----------------+---------------+------------------------------+
| Benchmark      | masterresults | issue22results               |
+================+===============+==============================+
| trivial commit | 14.9 us       | 13.2 us: 1.12x faster (-11%) |
+----------------+---------------+------------------------------+
| max retries    | 16.0 us       | 15.1 us: 1.06x faster (-5%)  |
+----------------+---------------+------------------------------+
@jamadden
Copy link
Member Author

jamadden commented Sep 6, 2019

I need to release this project in order to make the necessary changes to nti.site. But I'd really like to see zopefoundation/transaction#84 merged and released before releasing this so that we can fix #17 and remove one of the XXX comments that explicit transactions are designed to help with in the first place.

@jzuech3
Copy link

jzuech3 commented Sep 6, 2019

I'd advocate we release this project as-is to get as much alpha burn in time as possible for these changes, coming back later for the transaction changes.

@jamadden
Copy link
Member Author

jamadden commented Sep 6, 2019

That's a fair point. The aborthook thing is pretty minor, and I think I've thought of a way to handle the XXX thing in a safe-enough way even without the transaction update; it's mostly a hypothetical issue anyway.

This lets us more confidently abort in more cases.

And know when we don't need to.
@jamadden jamadden merged commit 3cbf1dd into master Sep 6, 2019
@jamadden jamadden deleted the issue22 branch September 6, 2019 15:48
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.

New hooks to do things after setting transaction manager to explicit mode
2 participants