Skip to content

Add map access feature for lists - #36

Open
HRogge wants to merge 10 commits into
CZ-NIC:masterfrom
HRogge:fkie
Open

Add map access feature for lists#36
HRogge wants to merge 10 commits into
CZ-NIC:masterfrom
HRogge:fkie

Conversation

@HRogge

@HRogge HRogge commented Jan 9, 2020

Copy link
Copy Markdown
Contributor

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

llhotka commented Jan 13, 2020

Copy link
Copy Markdown
Member

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

HRogge commented Jan 16, 2020

Copy link
Copy Markdown
Contributor Author

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

llhotka commented Jan 16, 2020

Copy link
Copy Markdown
Member

@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

HRogge commented Jan 20, 2020

Copy link
Copy Markdown
Contributor Author

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

HRogge commented Jan 20, 2020

Copy link
Copy Markdown
Contributor Author

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

@llhotka

llhotka commented Jan 20, 2020

Copy link
Copy Markdown
Member

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

HRogge commented Jan 20, 2020

Copy link
Copy Markdown
Contributor Author

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

HRogge commented Mar 27, 2020

Copy link
Copy Markdown
Contributor Author

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

@llhotka

llhotka commented Mar 31, 2020

Copy link
Copy Markdown
Member

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

HRogge commented Apr 1, 2020

Copy link
Copy Markdown
Contributor Author

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

HRogge commented Apr 21, 2020

Copy link
Copy Markdown
Contributor Author

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

@llhotka

llhotka commented Apr 21, 2020

Copy link
Copy Markdown
Member

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

@HRogge

HRogge commented Apr 22, 2020

Copy link
Copy Markdown
Contributor Author

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