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 Caching mechanism (#2547) #2552

Open
wants to merge 16 commits into
base: maintenance/mps20223
Choose a base branch
from

Conversation

dbinkele
Copy link
Member

@dbinkele dbinkele commented Oct 9, 2024

solves #2547

@alexanderpann alexanderpann self-requested a review October 10, 2024 09:11
Copy link
Member

@alexanderpann alexanderpann left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! I just had a quick look:

  • You have to somehow derive the JBR executable from
    downloadJbr {
    to not duplicate the logic.
  • I really do not like seeing code for locking in our platform. It is so easy to use your API incorrectly. Can you implement is similar like here with a runnable parameter? I am not 100% sure, but I believe marking the boolean variable as volatile should also be enough.

btw, you can use new singleton() from the collections language.

@dbinkele dbinkele requested a review from kbirken October 10, 2024 12:12
@alexanderpann
Copy link
Member

@dbinkele I didn't understand your code with the locking at all, it doesn't look right to me. I pushed a commit. Can you see if the change would be okay for you?

@dbinkele
Copy link
Member Author

@dbinkele I didn't understand your code with the locking at all, it doesn't look right to me. I pushed a commit. Can you see if the change would be okay for you?

@alexanderpann This changes the semantics. The lock was there to make sure that Thread A is running in the 'withCachedLogicalChildren'-Environment and any Thread B who wants to do so as well must wait until A is finished. But both would do the caching. Now only does the caching, B starts without waiting and probably does no caching ....

@alexanderpann
Copy link
Member

alexanderpann commented Oct 14, 2024

@dbinkele B does wait, I added another test case but I can't show a counter example where it doesn't work. The current thread blocks until it calls unlock. You can check the output of the second test. "Start" is called at a random time but "Inside" and "End" are always executed in order.

@alexanderpann alexanderpann force-pushed the feature/Enable_Caching_of_ILogicalChildren_at_ILogicalChildOwner_#2547 branch from da97983 to 22b5843 Compare October 14, 2024 20:13
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.

2 participants