zyedidia / micro

A modern and intuitive terminal-based text editor
https://micro-editor.github.io
MIT License
24.4k stars 1.16k forks source link

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

Open dmaluka opened 3 months ago

dmaluka commented 3 months ago

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: 828871ac OS: any Terminal: any

JoeKar commented 3 months ago

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 commented 3 months ago

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).