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

pit-cs: Remove extra goroutine and Mutex #40

Merged
merged 2 commits into from May 14, 2022
Merged

Conversation

zjkmxy
Copy link
Member

@zjkmxy zjkmxy commented May 12, 2022

Ref: #38

@pulsejet
Copy link
Contributor

This deadlocks for some reason, but only when I have multiple faces/producers. Any ideas why?

@zjkmxy
Copy link
Member Author

zjkmxy commented May 13, 2022

I have no idea. Could you show me how you run it?

@pulsejet
Copy link
Contributor

Just running two ndnputchunks on different prefixes using a 1G file. Then two ndncatchunks in parallel for both the files.

@zjkmxy
Copy link
Member Author

zjkmxy commented May 13, 2022

Thanks. Let me have a try.

@zjkmxy
Copy link
Member Author

zjkmxy commented May 13, 2022

Unfortunately I cannot reproduce this on my computer.

One thing you can try is to add a buffer to the channel:

pitCs.updateTimer = make(chan struct{}, 5)

and see if that solves the problem.

If the problem is still there, you could add a core.LogDebug to Update() and AfterFunc, and send the log to me.

@zjkmxy
Copy link
Member Author

zjkmxy commented May 13, 2022

Another possible reason could be that there are too many expiring entries that blocks the channel expiringPitEntries.
I cannot reproduce it because my Mac is slow and this case never happens.
Let me try to fix this.

@zjkmxy
Copy link
Member Author

zjkmxy commented May 13, 2022

Hello. I pushed another commit. Please see if it fixes the problem.

@pulsejet
Copy link
Contributor

@zjkmxy doesn't lock up for me anymore, but I'm trying to figure out why performance is 25-50% worse. Right now it seems to me that map deletes are simply slow, so doing them on the same thread makes it slower.

But that's unrelated to this PR, so this looks good to me.

@zjkmxy zjkmxy merged commit 79b7e56 into named-data:master May 14, 2022
@zjkmxy
Copy link
Member Author

zjkmxy commented May 14, 2022

@zjkmxy doesn't lock up for me anymore, but I'm trying to figure out why performance is 25-50% worse. Right now it seems to me that map deletes are simply slow, so doing them on the same thread makes it slower.

But that's unrelated to this PR, so this looks good to me.

OK. Let me see if there is solution.
BTW, I guess ndnputchunks does not provide PIT Token? It seems that PITToken is not used when I run the scenario.

@pulsejet
Copy link
Contributor

Right ... maybe I need to stop testing with ndnputchunks

What I currently observe is that if I comment the following line then the aggregate throughput with two instances of putchunks-catchunks, the throughput (content store off) goes from ~3gbps to ~4.5gbps

delete(curNode.parent.children, curNode.component.String())

Though I realise now that this could simply be because of the increased GC overhead ...

@zjkmxy
Copy link
Member Author

zjkmxy commented May 14, 2022

Right ... maybe I need to stop testing with ndnputchunks

Another possibility is YaNFD does not use PIT Token due to bugs. I'm not sure if this is the case.

What I currently observe is that if I comment the following line then the aggregate throughput with two instances of putchunks-catchunks, the throughput (content store off) goes from ~3gbps to ~4.5gbps

delete(curNode.parent.children, curNode.component.String())

Though I realise now that this could simply be because of the increased GC overhead ...

Then, probably using a memory pool for PIT-CS table could be a better choice.
Also, I notice

pitTokenMap map[uint32]*nameTreePitEntry

This is wrong. One should use linear data structure. If we have a memory pool for PIT-CS entries, we can simply use the node id.

The implementation of csMap

csMap map[uint64]*nameTreeCsEntry

Also does not look good to me. I don't fully understand why it is used. And
index := p.hashCsName(data.Name())

This seems to assume there is no hash conflict, which could be wrong.

@zjkmxy
Copy link
Member Author

zjkmxy commented May 16, 2022

Since this PR is closed. Let's continue further discussion (if there is) on #42.

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.

None yet

2 participants