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

Weak reference based persistence context for streaming queries #2411

Merged
merged 7 commits into from
Oct 12, 2021

Conversation

rbygrave
Copy link
Member

The concept is to use this for streaming queries rather than periodically creating a new persistence context

@rbygrave
Copy link
Member Author

FYI @rPraml

@rPraml
Copy link
Contributor

rPraml commented Oct 11, 2021

Hello Rob, I took a look at this PR and combined it with the ideas here: #2407 (comment)

and this is my result: https://github.com/FOCONIS/ebean/commits/feature/weak-reference-persistence-context

here I removed the whole copy stuff that came with #2021 / #2028 - but the persistence context knows, when we are in a findEach loop.
In this case, the classContext puts beanReferences instead direct references in the map.
So in short words, the persistence context can switch between "hard" and "weak" references.

This change would also allow to pass beans out of a fineEach loop now
(see what I mean FOCONIS@3f5009a#diff-89fa095ae86bf0579201a40a6fe845b17b8f64e4e333c930fac79740a05b04cbR153 - I know this is probably a bad programming style - but it will work now ;) )

Estimation of performance impact:

  • ONE ReferenceQueue is created by default per context (and not per ClassContext). There is not much special in ReferenceQueue, so it should not be measurable. Can also be changed to do that lazy on first beginIterate()
  • The DefaultPersistenceContext calls expungeState very often. This will effectively poll the queue, which is implemented like this:
    public Reference<? extends T> poll() {
        if (head == null)
            return null;
        synchronized (lock) {
            return reallyPoll();
        }
    }

So if nothing is in queue (which is the case, unless we switch to reference mode) - this is just a simple null check.

  • An additional if (ret instanceof BeanRef) is done on all get operations. This should also be handled also very fast by the JVM

So in my mind, this should not slow down anything in findList etc. - the foreach loop may be even be faster, because no copy from parent context needs to be done.

@rPraml
Copy link
Contributor

rPraml commented Oct 11, 2021

@rbygrave please take a look at DLoadContext.resetPersistenceContext as it will not be called now
FOCONIS@2f27793#diff-fea955a5fb63b63407f0e00456db9229db36ac58155570bdfe81b7f7a79a9099L255
Could this lead to a problem?

As far as I see, the DLoadBean/ManyContext handles forEach internally. So this should be OK.

Cheers
Roland

…nce-context

Change DefaultPersistenceContect to use beginIterate / endIterate
@rbygrave rbygrave added this to the 12.12.3 milestone Oct 12, 2021
@rbygrave rbygrave merged commit 349cd01 into master Oct 12, 2021
@rbygrave
Copy link
Member Author

Thanks @rPraml , I think we have ended up with a good result here.

rPraml added a commit to FOCONIS/ebean that referenced this pull request Oct 13, 2021
rPraml added a commit to FOCONIS/ebean that referenced this pull request Nov 3, 2021
@rbygrave rbygrave deleted the feature/weak-reference-persistence-context branch November 30, 2021 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants