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

Compress SafeSocketHandle.Unix booleans #112417

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

antonfirsov
Copy link
Member

There are 7 (+1 on Mac) bool fields in SafeSocketHandle.Unix which can be collapsed into a single byte field, saving 6 bytes per socket instance. The extra cost of the flag read/writes should be negligible.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@antonfirsov

This comment was marked as outdated.

@stephentoub
Copy link
Member

stephentoub commented Feb 11, 2025

saving 6 bytes per socket instance

Does it actually? If the fields were otherwise of a size divisible by eight, there's no difference on 64-bit between an extra byte field and an extra 7 boolean fields; the size of the object will still be rounded up to the next 8 byte boundary.

Unless there's a real savings, I'd rather keep the code simple.

@antonfirsov
Copy link
Member Author

@EgorBot -intel

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Net;
using System.Net.Sockets;

BenchmarkSwitcher.FromAssembly(typeof(SocketBench).Assembly).Run(args);

[MemoryDiagnoser]
public class SocketBench
{
    private Socket? _listener;
    private Socket? _receiver;
    private Socket? _sender;
    private CancellationTokenSource? _cts;
    private byte[] _sendBuffer = new byte[16];

    [Benchmark]
    public int Send() => _sender!.Send(_sendBuffer);

    [Benchmark]
    public async Task<int> SendAsync() => await _sender!.SendAsync(_sendBuffer, SocketFlags.None, default);

    [Benchmark]
    public void CreateClose()
    {
        Socket sock = new Socket(SocketType.Stream, ProtocolType.Tcp);
        sock.Dispose();
    }

    [GlobalSetup(Targets = [nameof(Send), nameof(SendAsync)])]
    public async Task Setup()
    {
        _listener = new Socket(SocketType.Stream, ProtocolType.Tcp);
        _listener.Bind(new IPEndPoint(IPAddress.IPv6Loopback, 0));
        IPEndPoint serverEp = (IPEndPoint)_listener.LocalEndPoint!;
        _listener.Listen();

        Task<Socket> acceptTask = _listener.AcceptAsync();

        _sender = new Socket(SocketType.Stream, ProtocolType.Tcp);
        await _sender.ConnectAsync(serverEp);

        _receiver = await acceptTask;
        _cts = new CancellationTokenSource();

        _ = RunServer();

        async Task RunServer()
        {
            byte[] rcvBuffer = new byte[16];
            try
            {
                while (!_cts.Token.IsCancellationRequested)
                {
                    await _receiver.ReceiveAsync(rcvBuffer, _cts.Token);
                }
            }
            catch (OperationCanceledException) { }
        }
    }

    [GlobalCleanup(Targets = [nameof(Send), nameof(SendAsync)])]
    public void Cleanup()
    {
        _cts?.Cancel();
        _receiver?.Dispose();
        _listener?.Dispose();
        _sender?.Dispose();
    }
}

@antonfirsov
Copy link
Member Author

antonfirsov commented Feb 11, 2025

The socket allocation cost went down 176 B -> 168 B, so it scales back the type's footprint by an 8 byte step.

@stephentoub if you think this is not worth the added complexity, I'll close this.

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs:144

  • The setter for IsNonBlocking does not handle the transition from non-blocking to blocking correctly, which could lead to unexpected socket behavior.
set { if (value) _flags |= Flags.NonBlocking; else _flags &= ~Flags.NonBlocking; }

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs:187

  • The change in IsDisconnected property behavior from a private setter to a flag check might lead to unintended side effects.
internal bool IsDisconnected => (_flags & Flags.IsDisconnected) != 0;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants