Skip to content

Clean up network interface and avoid a hashmap linear search#3239

Merged
stevenengler merged 5 commits intoshadow:mainfrom
stevenengler:netiface-inet-socket
Nov 28, 2023
Merged

Clean up network interface and avoid a hashmap linear search#3239
stevenengler merged 5 commits intoshadow:mainfrom
stevenengler:netiface-inet-socket

Conversation

@stevenengler
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler commented Nov 26, 2023

This cleans up the network interface and removes a linear hashmap search in the network interface's priority queue.

The network interface used to need to deal with both rust and C sockets, so it stored tagged pointers for CompatSocket objects. Now that the TCP socket is the only remaining C socket, and it has a rust wrapper LegacyTcpSocket, C sockets aren't used in the network interface anymore. The network interface now only stores boxed InetSocket objects. This also allows us to remove tagged pointers from our C code.

The hashmap in the priority queue used by the fifo qdisc now uses custom hash and eq implementations, which allows us to avoid a linear search through the hashmap which used the canonical handles to identify the same socket.

The new hash function is more expensive since it uses a real hash function (DefaultHasher as used by the stdlib HashMap), whereas the original glib code doesn't seem to actually use a hash function:

https://docs.gtk.org/glib/type_func.HashTable.new.html

If hash_func is NULL, g_direct_hash() is used.

https://github.com/GNOME/glib/blob/40081f9b627904c2506b6d3837334e4ce05052da/glib/ghash.c#L2483-L2487

guint
g_direct_hash (gconstpointer v)
{
  return GPOINTER_TO_UINT (v);
}

This PR contains a lot of changes along the rust/C boundary, but everything seems to still work so hopefully it's correct.

This may fix #3238.

Edit:

There doesn't seem to be much (if any) of a performance regression. The simulation results have remained unchanged. The following are from a slightly older version of this PR, but I don't expect it would make any difference.

run_time
transfer_time_5242880 exit
ram_simtime

Edit 2: One commit was split off into #3240.

@stevenengler stevenengler added the Tag: Performance Related to improving shadow's run-time label Nov 26, 2023
@stevenengler stevenengler self-assigned this Nov 26, 2023
@github-actions github-actions bot added the Component: Main Composing the core Shadow executable label Nov 26, 2023
@stevenengler stevenengler force-pushed the netiface-inet-socket branch 5 times, most recently from 8aae9af to b3e2f13 Compare November 27, 2023 03:55
Copy link
Copy Markdown
Member

@robgjansen robgjansen left a comment

Choose a reason for hiding this comment

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

🚀

We no longer add C sockets to the network interface, so there's no longer a
need to store `CompatSockets` as tagged pointers. We can just store rust
`InetSockets` instead.
Two `InetSocket`s are equal only if they represent the same socket object.
This allows us to avoid a linear search through the priority queue's hash map.
@stevenengler stevenengler merged commit 9f9f1dd into shadow:main Nov 28, 2023
@stevenengler stevenengler deleted the netiface-inet-socket branch November 28, 2023 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Main Composing the core Shadow executable Tag: Performance Related to improving shadow's run-time

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants