-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Move the access control ownership and initialization to Server. #14680
Move the access control ownership and initialization to Server. #14680
Conversation
PR #14680: Size comparison from 9f16e9a to 8ded51f Increases above 0.2%:
Increases (36 builds for cyw30739, efr32, esp32, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (9 builds for esp32, linux)
Full report (41 builds for cyw30739, efr32, esp32, linux, mbed, nrfconnect, p6, qpg, telink)
|
Just as a note, this is adding several KB or RAM use. We should probably have followup issue to reduce that... |
We do already have issue #14450 for general Access Control performance (all aspects), @bzbarsky-apple is that sufficient or do we also want a more targeted/specific issue? |
I'm currently looking into the cause of the size differences to see if it's something not already covered by #14450. |
@mlepage-google I would prefer a more targeted issue, for this and the increase from #14657 (which might be the same increase?). |
My investigations showed that the size increases for the apps I could check were generally due to the ACL code. It was previously removed from the build. Edit: Specifically the increase is from references to the ExampleAccessControlDelegate implementation. |
How could it have been removed from the build when it was used in the acl cluster? |
Problem
The simple ACL storage added in #14253 is only supported by Linux and MacOS. By moving the ownership of the AccessControl object to Server and using the platform storage delegate that it owns, all platforms that use Server will get ACL persistence.
Fixes #10251.
Change overview
Testing