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

(Option 2) Fixed O(N^2) performance when handling --bind flags #630

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

witaway
Copy link

@witaway witaway commented Apr 30, 2024

Issue #384. Same Pull Request as #629 (please read it). But I've been thinking about changes I have made and realized that maybe segment trees with lazy propagation is a little bit overengineering just to set flags. In this branch I've removed the whole segment tree data structure and replaced it with boolean arrays.

When segment tree uses lazy propagation to assign value on the interval and uses recursive algorithm to retrieve actual value on the index, current implementation just memset() the needed part of array to set flags and access needed flag by index. In all other respects, the algorithm remains unchanged.

  1. Technically here we revert back to O(N^2) complexity, but compiler optimizations and speed of memset performs also great on even big amounts of mount points. At the same time, if we run some more complex benchmarks, maybe with hundreds of thousands mount points, there's a great possibility that segment trees will perform better.

  2. At the same time, again, boolean array takes really much less memory than segment tree (4N) of integers. And it's a big advantage. Also, It's just more simple.

  3. With this approach we can also try to inline all the check/set flags function. I've tried. Nothing changed. Maybe it became a little slower. Anyway, there's such possibility.

  4. It's a way more simple to understand and debug

My benchmark with memset approach:

flatpak run com.microsoft.Edge
Without optimization => memset approach => segtree approach

  1. 0.013712 => 0.003204 => 0.004324
  2. 0.040225 => 0.007098 => 0.006671
  3. 0.004751 => 0.002331 => 0.002611

A test where I pass bwrap 2000 "--bind" flags:
bwrap --dev-bind / / --bind /usr /tmp/1 --bind /usr /tmp/2 ... --bind /usr /tmp/2000 /bin/true

  1. No optimizations: 14.038314
  2. Segtree approach: 0.218774
  3. Memset approach: 0.362339

Yahor Levanenka added 4 commits April 30, 2024 01:04
More advanced explanation of algorithm placed it bind_mount_fixup(comments
1. Enabled the usage of more safe alternatives to malloc and strdup;
2. Improved debug messages;
3. Fixed some memory leaks with usage of cleanup
4. Make comments more clear and simple to understand
5. Removed elapsed time output (added it before for comparing with old realization)
6. Improved code-style a little
May be a breaking change, so I changed the version.
Disabled DEBUG mode
@witaway
Copy link
Author

witaway commented Apr 30, 2024

Maybe we should add parameter like --many-binds to switch the algorithm, but idk, sounds like a delusion. Right now I cannot imagine the use case :D

@thomasjm
Copy link

I'm not a maintainer but it looks like your editor introduced a bunch of whitespace/formatting changes? I imagine they'd like to see the PR without those.

@witaway
Copy link
Author

witaway commented Apr 30, 2024

@thomasjm Thank you, I'll try to fix it

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