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

Add map access feature for lists #36

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Add map access feature for lists #36

wants to merge 15 commits into from

Conversation

HRogge
Copy link
Contributor

@HRogge HRogge commented Jan 9, 2020

This change allows to access children of InstanceNodes to be accessed by a tuple containing the keys defined in the yang schema:

instancenode[(1,'hello')]

for a yang structure like

list justAlist {
        key "index name";
        leaf index {
            type uint16;
        }
        leaf name {
            type string;
        }
        .....

to increase performance the mapping between the list and its children is calculated and stored during first use.

@llhotka
Copy link
Member

llhotka commented Jan 13, 2020

This change means that all list entries have to be visited upon the first access, whereas with the current code only half of the entries (on the average) need to be visited.

Perhaps it would be better to implement _childmap as a cache: do the sequential lookup for the first access and write all keys along the way to self._childmap, but stop immediately when the entry is found. With a next access, the entry would either be served from the cache, or the sequential lookup will continue from the place where it ended last time.

@HRogge
Copy link
Contributor Author

HRogge commented Jan 16, 2020

I see your point, the problem is that if you have only a partial cache, each query for a non-existing key has to iterate over all elements. Using the key-access operator only takes double the time if you populate the cache fully, but all consecutive accesses will be very fast (both hit and miss).

My current problem is that the cache needs to be cleared/updated every times a node member gets added, removed or changed... and I am still trying to work out on which code paths this would happen.

@llhotka
Copy link
Member

llhotka commented Jan 16, 2020

@HRogge my idea was to scan only entries that haven't been scanned yet. That would mean also keeping the index of the last-scanned entry.

@HRogge
Copy link
Contributor Author

HRogge commented Jan 20, 2020

I like this idea... I will see that I modify my path and send a new pull request so that you can review it. The only thing missing then would be to clear/modify the index if something modifies the keys of a subnode.

@HRogge
Copy link
Contributor Author

HRogge commented Jan 20, 2020

I have changed the caching lookup to the one you suggested and also added a "map" based lookup and some convenience functions.

@llhotka
Copy link
Member

llhotka commented Jan 20, 2020

Looks good, thanks. I will add some tests and update docs, and then make a new version.

BTW, do you have any bigger data to demonstrate a speedup?

@HRogge
Copy link
Contributor Author

HRogge commented Jan 20, 2020

No... but I always worry that I break existing stuff by introducing some nice O(n^2) delay... it would also work without the cache...

@HRogge
Copy link
Contributor Author

HRogge commented Mar 27, 2020

Any new thoughts about merging this changes? Anything I should change on the code before this happens?

@llhotka
Copy link
Member

llhotka commented Mar 31, 2020

Actually, this change also has to be reflected in methods that manipulate the instance, like insert_before and insert_after, and possibly elsewhere, because otherwise the cache may get out of sync. Unfortunately, I haven't had time for hacking lately

@HRogge
Copy link
Contributor Author

HRogge commented Apr 1, 2020

I will create a branch without the cache and send another pull request. I still think the cache could be useful, but you are right that I might have missed some parts where the instance data can be modified.

@HRogge
Copy link
Contributor Author

HRogge commented Apr 21, 2020

I have removed the mapping cache for now, maybe you want to have a look again?

@llhotka
Copy link
Member

llhotka commented Apr 21, 2020

Hmm, I don't see anything new, the last commit in this PR is still from 20 January.

@HRogge
Copy link
Contributor Author

HRogge commented Apr 22, 2020

Sorry, I pushed the wrong branch... sigh

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