Skip to content

rfc: clean it up and optimize#62

Merged
zjkmxy merged 16 commits intonamed-data:masterfrom
pulsejet:varun/clean
Dec 6, 2024
Merged

rfc: clean it up and optimize#62
zjkmxy merged 16 commits intonamed-data:masterfrom
pulsejet:varun/clean

Conversation

@pulsejet
Copy link
Collaborator

@pulsejet pulsejet commented Dec 6, 2024

My intention is to rip off everything that isn't or is hard to maintain, and hopefully make this more usable.

  1. Remove ethernet face
  2. Remove face events
  3. Remove the UI
  4. Remove NDNLP reliability and retransmission
  5. Completely rip off the ndn library here, switch to go-ndn instead

Various optimizations

  1. Switch to sync.Map
  2. GC optimizations
  3. Remove pointless goroutines

Benchmark

  1. GOMAXPROCS=4 GOGC=400 ./yanfd with one instance of putchunks and 2 instances of catchunks.
  2. Goodput goes from ~3gbps to ~4.5gbps

@zjkmxy
Copy link
Member

zjkmxy commented Dec 6, 2024

Thank you for your help. This relieves the maintenance burden. But I want to make sure that:

  • What is the goal of YaNFD?
  • Do we still want to support the Windows App?

func (f *FibStrategyTree) fillTreeToPrefixEnc(name enc.Name) *fibStrategyTreeEntry {
curNode := f.root.findLongestPrefixEntryEnc(name)
for depth := curNode.depth + 1; depth <= len(name); depth++ {
component, _ := enc.ComponentFromBytes(At(name, depth-1).Bytes())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a deep copy. I'm not really sure what is going on here: the underlying array gets deallocated / overwritten somehow if this isn't copied and it becomes garbage. If I remove the copies in this file:

root@0037b98ec2ac:/work/YaNFD# nfdc strategy set /svs/abcdefghijklmnopqrstuvwxyz /localhost/nfd/strategy/multicast
strategy-set prefix=/svs/abcdefghijklmnopqrstuvwxyz strategy=/localhost/nfd/strategy/multicast/v=1
root@0037b98ec2ac:/work/YaNFD# nfdc strategy set /svs/abcdef /localhost/nfd/strategy/multicast
strategy-set prefix=/svs/abcdef strategy=/localhost/nfd/strategy/multicast/v=1
root@0037b98ec2ac:/work/YaNFD# nfdc strategy list
prefix=/ strategy=/localhost/nfd/strategy/best-route/v=1
prefix=/%85h%0E/abcdef strategy=/localhost/nfd/strategy/multicast/v=1
prefix=/%85h%0E/abcdefk%27%07%25%08%09localhost%08%03nfd strategy=/tegy%08%09mul/cas/%20J%07%9F%D4t%E9%03/I%9C%F4Yr%99%A2oT/v=1
root@0037b98ec2ac:/work/YaNFD# 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for clarifying. This seems to be a big issue. Maybe we should add a comment explaining this.

Copy link
Member

@yoursunny yoursunny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ethernet face was last maintained in 2024-Aug and is not broken so it should be kept.
No objection in deleting other features.

@pulsejet
Copy link
Collaborator Author

pulsejet commented Dec 6, 2024

Thanks for reviewing, Xinyu.

  • What is the goal of YaNFD?

I've more stuff planned for the future (which might not materialize). Essentially I want YaNFD to be the out-of-the box NDN daemon that's focuses on ease of use and is maintanable. I'll elaborate in a Saturday call if you're available.

  • Do we still want to support the Windows App?

For now, I don't see the point. Some of the stuff that is removed here can be pulled back in later. My priority for this PR is to nuke the YaNFD/ndn package.

@pulsejet
Copy link
Collaborator Author

pulsejet commented Dec 6, 2024

Ethernet face was last maintained in 2024-Aug and is not broken so it should be kept.

If it works then it's probably fine; I'll try to pull this back in.

@Pesa
Copy link
Member

Pesa commented Dec 6, 2024

If it works then it's probably fine; I'll try to pull this back in.

https://www.youtube.com/watch?v=UPw-3e_pzqU

@zjkmxy
Copy link
Member

zjkmxy commented Dec 6, 2024

  • Do we still want to support the Windows App?

For now, I don't see the point. Some of the stuff that is removed here can be pulled back in later. My priority for this PR is to nuke the YaNFD/ndn package.

Then let it die 🙈
TBH I'm kind of tired of satisfying Windows store's policy, which seems to be more strict than COVID days

If it works then it's probably fine; I'll try to pull this back in.

Let's do it in a separate PR. This is already too large and it seems to be working.

@zjkmxy zjkmxy merged commit 3d5acdf into named-data:master Dec 6, 2024
6 checks passed
@pulsejet pulsejet deleted the varun/clean branch December 7, 2024 17:16
pulsejet pushed a commit that referenced this pull request Dec 24, 2024
rfc: clean it up and optimize
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.

4 participants