-
Notifications
You must be signed in to change notification settings - Fork 879
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
Fixes mojo max size problem #4948
Conversation
As noted on Slack, I don't think this solution is correct Ideal solution would be either:
|
@bsclifton this one is just testing if this is a problem or not. It's not a final fix |
For sure 😄 Just left the note in case I was going to sleep before the conclusion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed with @NejcZdovc - intention of this PR is to "stop the bleeding"
The limits here are much lower. We believe the Banner limit is what was causing a problem. We talked with teammates and determined that max publisher record size is 104 bytes (each) and max banner record size (each) is 1,543.
With limit set (before this PR) of 150,000... this results in 231,450,000 which is larger than the max size (GetConfiguration().max_message_num_bytes
) which is 134,217,728. New size (max record count 80,000) is 123,440,000 (not counting the mojo message details)
There of course will be extra bytes since the messages themselves are encoded. This needs to be determined and accounted for
This PR puts us under the safe limits for both. Impact will be verified list takes longer to be realized- but it will stop the CPU problem and crashing problem (on Nightly and Beta)
When PR builder finishes, we can verify the binary that is created to ensure problem is gone 👍
Thanks to @mkarolin for finding the |
confirming that CPU is not a problem anymore after this fix |
Fixes mojo max size problem
Cherry-picked to 1.7 with 0ddb199 |
Resolves brave/brave-browser#8691
Submitter Checklist:
npm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Reviewer Checklist:
After-merge Checklist:
changes has landed on.