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

[performance] A lot of CPU time consumed by accesses to buf.Settings map #3228

Open
dmaluka opened this issue Apr 4, 2024 · 2 comments
Open

Comments

@dmaluka
Copy link
Collaborator

dmaluka commented Apr 4, 2024

CPU profiling via micro -profile shows that among the CPU time consumed by micro, a large fraction of time is usually spent in Go map accesses, e.g. in runtime.mapaccess1_faststr() and runtime.mapaccess2_faststr().

Most probably most of those map accesses are accesses to a buffer's Settings map. Micro's code, including such frequently called parts as displayBuffer(), is full of things like if b.Settings["diffgutter"].(bool). If we convert all those map accesses to simple struct field accesses (e.g. b.diffgutter), it should work much faster. (And we may still keep the map representation as well, e.g. for compatibility with plugins, just not use it inside micro itself all the time.)

Commit hash: 828871a
OS: any
Terminal: any

@JoeKar
Copy link
Collaborator

JoeKar commented Apr 4, 2024

If we convert all those map accesses to simple struct field accesses (e.g. b.diffgutter), it should work much faster. (And we may still keep the map representation as well, e.g. for compatibility with plugins, just not use it inside micro itself all the time.)

Hm, but only for a subset or am I wrong? Otherwise we maintain all settings twice. At least I don't know any representation of such an map in a transparent way as simple structure members...
The map definitely has one large pro, when it comes to the option lookup inside the infopane for the autocompletion.

@dmaluka
Copy link
Collaborator Author

dmaluka commented Apr 4, 2024

Otherwise we maintain all settings twice.

Yes, we maintain all settings twice, that's the idea. When setting an option value, we update both the struct field and the map, but when getting its value (which we do much more often), we only need to read the struct field.

At least I don't know any representation of such an map in a transparent way as simple structure members...

There is none, so we'd need to maintain both representations. But while we're at it, I guess we can use Go reflection to convert between both representations more or less automatically.

The map definitely has one large pro, when it comes to the option lookup inside the infopane for the autocompletion.

Yes, for option name validation, autocompletion etc we need a list of option name strings, in one form or another, not necessarily as a map. But for compatibility with plugins that read an option value directly from the map as bp.Buf.Settings["foo"] we need it to be a map. (BTW it seems this means we cannot make it compatible with those plugins that try to write it directly via bp.Buf.Settings["foo"] = "bar", if there are any).

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

No branches or pull requests

2 participants