Skip to content

flyweight permissions in server#5556

Merged
joshmoore merged 7 commits intoome:developfrom
mtbc:flyweight-permissions
Jan 16, 2018
Merged

flyweight permissions in server#5556
joshmoore merged 7 commits intoome:developfrom
mtbc:flyweight-permissions

Conversation

@mtbc
Copy link
Copy Markdown
Member

@mtbc mtbc commented Oct 20, 2017

What this PR does

Introduces a cache for the Boolean arrays associated with permissions restrictions. Then, the server should not hold on for long to many different arrays that represent the same permissions.

Testing this PR

There should be no CI regressions. Watch for differences in server speed, e.g., running time of integration tests. With #5586 reports from each thread's BooleanArrayCache every fifty minutes can be seen on eel in /opt/hudson/workspace/OMERO-DEV-merge-{deploy,integration}/src/dist/var/log/Blitz-0.log.

Related reading

https://trello.com/c/SXxKgAC3/8-flyweight-permissions-in-server

@mtbc mtbc changed the title flyweight permissions flyweight permissions in server Oct 20, 2017
@mtbc mtbc added the develop label Oct 20, 2017
@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Oct 25, 2017

Since I opened this PR integration test runtimes may be slightly improved, not much difference really.

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Oct 26, 2017

BasicACLVoter.postProcess may warrant investigation regarding http://trac.openmicroscopy.org/ome/ticket/9505#comment:16.

@mtbc mtbc added the ON_HOLD label Nov 17, 2017
@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Nov 20, 2017

#309 and #369 add integration testing that I think helps prevent regressions due to my adjustment of copyRestrictions.

@mtbc mtbc removed the ON_HOLD label Nov 20, 2017
@joshmoore
Copy link
Copy Markdown
Member

Having suggested the change, I have no particular issue with having this done, but I do wonder if we have any evidence for or against the performance improvement (time or space) of this change.

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Nov 22, 2017

I've not been able to discern any difference outside the error bars of noise, though it might be interesting to have the PR in partly for that it does throw when it detects that the array has been mutated since it last saw it.

@joshmoore
Copy link
Copy Markdown
Member

Have you watched the behavior for a running server? What size is typical for cacheRef? How much churn is there (hits/misses)?

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Nov 22, 2017

I have a bit but since forgot I'm afraid! There are certainly some hits. 😃 Want me to open an additional temporary PR that adds some monitoring for those?

@joshmoore
Copy link
Copy Markdown
Member

Could we perhaps get some numbers in a logging statement, perhaps at INFO initially but then can move back to DEBUG?

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Nov 22, 2017

When to log? Every many-minutes or something? Maybe we don't have to introduce another config property.

@joshmoore
Copy link
Copy Markdown
Member

Perhaps every 100 modifications or similar?

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Nov 28, 2017

Given that a running system seems able to generate thousands of cache hits per second and that logging implies statistics-gathering which means more state that needs concurrency protection, my plan tomorrow is to shift the cache to being thread-local.

@mtbc mtbc force-pushed the flyweight-permissions branch from aa81ca6 to 6b5a3ff Compare November 29, 2017 10:57
@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Nov 29, 2017

(Of course, the size of the cache is how many misses there were.)

@mtbc mtbc force-pushed the flyweight-permissions branch from 52ae3d7 to 0af0543 Compare November 29, 2017 12:58
@joshmoore
Copy link
Copy Markdown
Member

Pasted some initial output to gh-5586. Looks good. Knowing that Counter can grow quite large, should counting be disabled if no logging will take place (with the caveat that it would then need to be restarted if logging is enabled at runtime).

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Dec 5, 2017

I would say that Counter performs well enough in time and space that it is fine to leave it going regardless of logging.

@mtbc
Copy link
Copy Markdown
Member Author

mtbc commented Dec 5, 2017

Agreeably I am also not seeing any "cache violation" errors being thrown by BooleanArrayCache among eel's server logs.

@mtbc mtbc added this to the 5.4.2 milestone Dec 12, 2017
@joshmoore joshmoore merged commit f4c9fbc into ome:develop Jan 16, 2018
@mtbc mtbc deleted the flyweight-permissions branch January 16, 2018 08:41
rgozim pushed a commit to rgozim/openmicroscopy that referenced this pull request Oct 8, 2018
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