JSBSim-Team / jsbsim

An open source flight dynamics & control software library
GNU Lesser General Public License v2.1
1.22k stars 420 forks source link

Use cached PropertyNodes in python API for property reads #993

Closed seanmcleod closed 5 months ago

seanmcleod commented 5 months ago

Based on performance tests in https://github.com/JSBSim-Team/jsbsim/discussions/977 implement the PropertyNode caching in the Python API for property reads.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a5855b6) 24.87% compared to head (ce47e42) 24.87%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #993 +/- ## ======================================= Coverage 24.87% 24.87% ======================================= Files 168 168 Lines 18908 18908 ======================================= Hits 4704 4704 Misses 14204 14204 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

seanmcleod commented 5 months ago

Yep, key hashing definitely faster than property name lookup, plus we were doing 2 property name lookups/traversals. Initially via pm.hasNode(_key) which was discarding the node pointer and simply returning a bool, and then another property name lookup during the call to GetPropertyValue(name). Plus I guess a couple of transitions from python code to JSBSim C++ code and back as well.

    def __getitem__(self, key: str) -> float:
        _key = key.strip()
        pm = self.get_property_manager()
        if not pm.hasNode(_key):
            raise KeyError("No property named {}".format(_key))
        return self.get_property_value(_key)

    def get_property_value(self, name: str) -> float:
        """@Dox(JSBSim::FGFDMExec::GetPropertyValue) """
        return self.thisptr.GetPropertyValue(name.encode())
bcoconni commented 5 months ago

Yes the code of __getitem__ was implemented before FGPropertyNode was included in the Python binding. At that time, the problem was that get_property_value always returns a value whether the property exists or not. So to inform users if they were requesting the value of a property that did not exist, the existence was first tested via has_node prior to calling get_property_value.

Indeed, now that FGPropertyNode is available the sequence of testing the existence of the property then returning the value of that property is now feasible with a single name lookup, which is faster. And it is even faster with a cache. Your patch hits 2 birds with a stone 🐦 👍

seanmcleod commented 5 months ago

Ah, was wondering why it was coded that way.

I'm used to the more aggressive form, "to kill two birds with one stone" :wink:

bcoconni commented 4 months ago

I'm used to the more aggressive form, "to kill two birds with one stone" :wink:

Well, I'll try to pretend my error was made on purpose because aviation as a whole needs to be friendlier to the environment 😄