micro-optimizations for is_windows() et al. #14206
Conversation
bruchar1
requested review from
dcbaker,
mensinda,
xclaesse and
jpakkane
as code owners
bruchar1
force-pushed
the
optimize-is_windows-et-al
branch
from
93c19fa to
903c3ab
Compare
| def is_windows() -> bool: | ||
| platname = platform.system().lower() | ||
| return platname == 'windows' | ||
|
|
There was a problem hiding this comment.
We could also just make this a global constant just like _in_ci, no need to make it specifically be a class attribute on the logger.
There was a problem hiding this comment.
Same with all the rest, really.
bruchar1
force-pushed
the
optimize-is_windows-et-al
branch
from
903c3ab to
cadbdcd
Compare
mesonbuild/envconfig.py
Outdated
| @lazy_property | ||
| def is_windows(self) -> bool: | ||
| """ | ||
| Machine is windows? | ||
| """ | ||
| return self.system == 'windows' |
There was a problem hiding this comment.
In particular, all the envconfig functions literally translate a string comparison against a single property to a named function.
There was a problem hiding this comment.
So, what do you suggest? To drop that commit, or to precompute them in the __init__ function?
There was a problem hiding this comment.
We could also just make this a global constant just like _in_ci, no need to make it specifically be a class attribute on the logger.
Same with all the rest, really.
Here, it didn't used to be a global constant, and the point is just that if we're going to convert it from a method to a property (lazy or not) we might as well make it a precomputed property
I'm assuming the string comparison doesn't show up in profiling, but tons of function calls do... Performing a small handful of string comparisons at startup should be really cheap.
There was a problem hiding this comment.
It is better like that? If so, I will just squash the 2 commits.
bruchar1
force-pushed
the
optimize-is_windows-et-al
branch
from
cadbdcd to
e16bbee
Compare
| @@ -350,7 +350,7 @@ def get_target_filename_for_linking(self, target: T.Union[build.Target, build.Cu | |||
| if isinstance(target, build.SharedLibrary): | |||
| link_lib = target.get_import_filename() or target.get_filename() | |||
| # In AIX, if we archive .so, the blibpath must link to archived shared library otherwise to the .so file. | |||
| if mesonlib.is_aix() and target.aix_so_archive: | |||
| if mesonlib.Platform.is_aix and target.aix_so_archive: | |||
There was a problem hiding this comment.
This is sorta tangental, but I'm not sure the use of meson.is* is actually correct here, I think this needs to be Environment.machines[target.for_machine].is_aix
There was a problem hiding this comment.
Maybe cross-compiling to AIX is less common than cross compiling between Windows and Linux. :D
|
Very side note here, the only place where it's guaranteed to be correct to use the |
This micro-optimization will cache the result of is_windows() and other similar functions.
bruchar1
force-pushed
the
optimize-is_windows-et-al
branch
from
ac23f65 to
249f924
Compare
It is better like that? If so, I will squash it with the other commit
bruchar1
force-pushed
the
optimize-is_windows-et-al
branch
from
249f924 to
9ae4669
Compare
Use the
lazy_propertydecorator to replace theis_windows()function with attribute lookup. Since those functions are used everywhere in many loops, this may lead to a small performance improvement.