tweaks to NAMES implementation (#2058)

* tweaks to NAMES implementation

* tweak member caching

* add a benchmark for NAMES

irc/channel.go

@@ -34,7 +34,6 @@ type Channel struct {
 	key               string
 	forward           string
 	members           MemberSet
-	membersCache      []*Client // allow iteration over channel members without holding the lock
 	name              string
 	nameCasefolded    string
 	server            *Server
@@ -54,6 +53,9 @@ type Channel struct {
 	dirtyBits         uint
 	settings          ChannelSettings
 	uuid              utils.UUID
+	// these caches are paired to allow iteration over channel members without holding the lock
+	membersCache    []*Client
+	memberDataCache []*memberData
 }
 
 // NewChannel creates a new channel from a `Server` and a `name`
@@ -421,16 +423,19 @@ func (channel *Channel) AcceptTransfer(client *Client) (err error) {
 
 func (channel *Channel) regenerateMembersCache() {
 	channel.stateMutex.RLock()
-	result := make([]*Client, len(channel.members))
+	membersCache := make([]*Client, len(channel.members))
+	dataCache := make([]*memberData, len(channel.members))
 	i := 0
-	for client := range channel.members {
+	for client, info := range channel.members {
-		result[i] = client
+		membersCache[i] = client
+		dataCache[i] = info
 		i++
 	}
 	channel.stateMutex.RUnlock()
 
 	channel.stateMutex.Lock()
-	channel.membersCache = result
+	channel.membersCache = membersCache
+	channel.memberDataCache = dataCache
 	channel.stateMutex.Unlock()
 }
 
@@ -438,6 +443,8 @@ func (channel *Channel) regenerateMembersCache() {
 func (channel *Channel) Names(client *Client, rb *ResponseBuffer) {
 	channel.stateMutex.RLock()
 	clientData, isJoined := channel.members[client]
+	chname := channel.name
+	membersCache, memberDataCache := channel.membersCache, channel.memberDataCache
 	channel.stateMutex.RUnlock()
 	isOper := client.HasRoleCapabs("sajoin")
 	respectAuditorium := channel.flags.HasMode(modes.Auditorium) && !isOper &&
@@ -445,52 +452,32 @@ func (channel *Channel) Names(client *Client, rb *ResponseBuffer) {
 	isMultiPrefix := rb.session.capabilities.Has(caps.MultiPrefix)
 	isUserhostInNames := rb.session.capabilities.Has(caps.UserhostInNames)
 
-	maxNamLen := 480 - len(client.server.name) - len(client.Nick())
+	maxNamLen := 480 - len(client.server.name) - len(client.Nick()) - len(chname)
-	var namesLines []string
+	var tl utils.TokenLineBuilder
-	var buffer strings.Builder
+	tl.Initialize(maxNamLen, " ")
 	if isJoined || !channel.flags.HasMode(modes.Secret) || isOper {
-		for _, target := range channel.Members() {
+		for i, target := range membersCache {
+			if !isJoined && target.HasMode(modes.Invisible) && !isOper {
+				continue
+			}
 			var nick string
 			if isUserhostInNames {
 				nick = target.NickMaskString()
 			} else {
 				nick = target.Nick()
 			}
-			channel.stateMutex.RLock()
+			memberData := memberDataCache[i]
-			memberData, _ := channel.members[target]
+			if respectAuditorium && memberData.modes.HighestChannelUserMode() == modes.Mode(0) {
-			channel.stateMutex.RUnlock()
-			modeSet := memberData.modes
-			if modeSet == nil {
 				continue
 			}
-			if !isJoined && target.HasMode(modes.Invisible) && !isOper {
+			tl.AddParts(memberData.modes.Prefixes(isMultiPrefix), nick)
-				continue
-			}
-			if respectAuditorium && modeSet.HighestChannelUserMode() == modes.Mode(0) {
-				continue
-			}
-			prefix := modeSet.Prefixes(isMultiPrefix)
-			if buffer.Len()+len(nick)+len(prefix)+1 > maxNamLen {
-				namesLines = append(namesLines, buffer.String())
-				buffer.Reset()
-			}
-			if buffer.Len() > 0 {
-				buffer.WriteString(" ")
-			}
-			buffer.WriteString(prefix)
-			buffer.WriteString(nick)
-		}
-		if buffer.Len() > 0 {
-			namesLines = append(namesLines, buffer.String())
 		}
 	}
 
-	for _, line := range namesLines {
+	for _, line := range tl.Lines() {
-		if buffer.Len() > 0 {
+		rb.Add(nil, client.server.name, RPL_NAMREPLY, client.nick, "=", chname, line)
-			rb.Add(nil, client.server.name, RPL_NAMREPLY, client.nick, "=", channel.name, line)
 	}
-	}
+	rb.Add(nil, client.server.name, RPL_ENDOFNAMES, client.nick, chname, client.t("End of NAMES list"))
-	rb.Add(nil, client.server.name, RPL_ENDOFNAMES, client.nick, channel.name, client.t("End of NAMES list"))
 }
 
 // does `clientMode` give you privileges to grant/remove `targetMode` to/from people,
@@ -514,7 +501,7 @@ func channelUserModeHasPrivsOver(clientMode modes.Mode, targetMode modes.Mode) b
 // ClientIsAtLeast returns whether the client has at least the given channel privilege.
 func (channel *Channel) ClientIsAtLeast(client *Client, permission modes.Mode) bool {
 	channel.stateMutex.RLock()
-	memberData := channel.members[client]
+	memberData, present := channel.members[client]
 	founder := channel.registeredFounder
 	channel.stateMutex.RUnlock()
 
@@ -522,6 +509,10 @@ func (channel *Channel) ClientIsAtLeast(client *Client, permission modes.Mode) b
 		return true
 	}
 
+	if !present {
+		return false
+	}
+
 	for _, mode := range modes.ChannelUserModes {
 		if memberData.modes.HasMode(mode) {
 			return true
@@ -571,20 +562,20 @@ func (channel *Channel) setMemberStatus(client *Client, status alwaysOnChannelSt
 	}
 	channel.stateMutex.Lock()
 	defer channel.stateMutex.Unlock()
-	if _, ok := channel.members[client]; !ok {
+	if mData, ok := channel.members[client]; ok {
-		return
+		mData.modes.Clear()
+		for _, mode := range status.Modes {
+			mData.modes.SetMode(modes.Mode(mode), true)
+		}
+		mData.joinTime = status.JoinTime
 	}
-	memberData := channel.members[client]
-	memberData.modes = newModes
-	memberData.joinTime = status.JoinTime
-	channel.members[client] = memberData
 }
 
 func (channel *Channel) ClientHasPrivsOver(client *Client, target *Client) bool {
 	channel.stateMutex.RLock()
 	founder := channel.registeredFounder
-	clientModes := channel.members[client].modes
+	clientData, clientOK := channel.members[client]
-	targetModes := channel.members[target].modes
+	targetData, targetOK := channel.members[target]
 	channel.stateMutex.RUnlock()
 
 	if founder != "" {
@@ -595,7 +586,11 @@ func (channel *Channel) ClientHasPrivsOver(client *Client, target *Client) bool
 		}
 	}
 
-	return channelUserModeHasPrivsOver(clientModes.HighestChannelUserMode(), targetModes.HighestChannelUserMode())
+	return clientOK && targetOK &&
+		channelUserModeHasPrivsOver(
+			clientData.modes.HighestChannelUserMode(),
+			targetData.modes.HighestChannelUserMode(),
+		)
 }
 
 func (channel *Channel) hasClient(client *Client) bool {
@@ -1319,9 +1314,9 @@ func (channel *Channel) SendSplitMessage(command string, minPrefixMode modes.Mod
 
 	if channel.flags.HasMode(modes.OpModerated) {
 		channel.stateMutex.RLock()
-		cuData := channel.members[client]
+		cuData, ok := channel.members[client]
 		channel.stateMutex.RUnlock()
-		if cuData.modes.HighestChannelUserMode() == modes.Mode(0) {
+		if !ok || cuData.modes.HighestChannelUserMode() == modes.Mode(0) {
 			// max(statusmsg_minmode, halfop)
 			if minPrefixMode == modes.Mode(0) || minPrefixMode == modes.Voice {
 				minPrefixMode = modes.Halfop
@@ -1490,6 +1485,7 @@ func (channel *Channel) Purge(source string) {
 	chname := channel.name
 	members := channel.membersCache
 	channel.membersCache = nil
+	channel.memberDataCache = nil
 	channel.members = make(MemberSet)
 	// TODO try to prevent Purge racing against (pending) Join?
 	channel.stateMutex.Unlock()

irc/client_test.go

@@ -4,8 +4,10 @@
 package irc
 
 import (
+	"fmt"
 	"testing"
 
+	"github.com/ergochat/ergo/irc/languages"
 	"github.com/ergochat/ergo/irc/utils"
 )
 
@@ -30,6 +32,47 @@ func BenchmarkGenerateBatchID(b *testing.B) {
 	}
 }
 
+func BenchmarkNames(b *testing.B) {
+	channelSize := 1024
+	server := &Server{
+		name: "ergo.test",
+	}
+	lm, err := languages.NewManager(false, "", "")
+	if err != nil {
+		b.Fatal(err)
+	}
+	server.config.Store(&Config{
+		languageManager: lm,
+	})
+	for i := 0; i < b.N; i++ {
+		channel := &Channel{
+			name:           "#test",
+			nameCasefolded: "#test",
+			server:         server,
+			members:        make(MemberSet),
+		}
+		for j := 0; j < channelSize; j++ {
+			nick := fmt.Sprintf("client_%d", j)
+			client := &Client{
+				server:         server,
+				nick:           nick,
+				nickCasefolded: nick,
+			}
+			channel.members.Add(client)
+			channel.regenerateMembersCache()
+			session := &Session{
+				client: client,
+			}
+			rb := NewResponseBuffer(session)
+			channel.Names(client, rb)
+			if len(rb.messages) < 2 {
+				b.Fatalf("not enough messages: %d", len(rb.messages))
+			}
+			// to inspect the messages: line, _ := rb.messages[0].Line()
+		}
+	}
+}
+
 func TestUserMasks(t *testing.T) {
 	var um UserMaskSet
 

irc/handlers.go

@@ -2060,8 +2060,6 @@ func namesHandler(server *Server, client *Client, msg ircmsg.Message, rb *Respon
 		channels = strings.Split(msg.Params[0], ",")
 	}
 
-	// TODO: in a post-federation world, process `target` (server to forward request to)
-
 	// implement the modern behavior: https://modern.ircdocs.horse/#names-message
 	// "Servers MAY only return information about the first <channel> and silently ignore the others."
 	// "If no parameter is given for this command, servers SHOULD return one RPL_ENDOFNAMES numeric

irc/modes/modes.go

@@ -345,6 +345,10 @@ func NewModeSet() *ModeSet {
 	return &set
 }
 
+func (set *ModeSet) Clear() {
+	utils.BitsetClear(set[:])
+}
+
 // test whether `mode` is set
 func (set *ModeSet) HasMode(mode Mode) bool {
 	if set == nil {

irc/types.go

@@ -16,17 +16,16 @@ import (
 type ClientSet = utils.HashSet[*Client]
 
 type memberData struct {
-	modes    *modes.ModeSet
+	modes    modes.ModeSet
 	joinTime int64
 }
 
 // MemberSet is a set of members with modes.
-type MemberSet map[*Client]memberData
+type MemberSet map[*Client]*memberData
 
 // Add adds the given client to this set.
 func (members MemberSet) Add(member *Client) {
-	members[member] = memberData{
+	members[member] = &memberData{
-		modes:    modes.NewModeSet(),
 		joinTime: time.Now().UnixNano(),
 	}
 }

irc/utils/bitset.go

@@ -48,6 +48,13 @@ func BitsetSet(set []uint32, position uint, on bool) (changed bool) {
 	}
 }
 
+// BitsetClear clears the bitset in-place.
+func BitsetClear(set []uint32) {
+	for i := 0; i < len(set); i++ {
+		atomic.StoreUint32(&set[i], 0)
+	}
+}
+
 // BitsetEmpty returns whether the bitset is empty.
 // This has false positives under concurrent modification (i.e., it can return true
 // even though w.r.t. the sequence of atomic modifications, there was no point at

irc/utils/text.go

@@ -125,6 +125,28 @@ func (t *TokenLineBuilder) Add(token string) {
 	t.buf.WriteString(token)
 }
 
+// AddParts concatenates `parts` into a token and adds it to the line,
+// creating a new line if necessary.
+func (t *TokenLineBuilder) AddParts(parts ...string) {
+	var tokenLen int
+	for _, part := range parts {
+		tokenLen += len(part)
+	}
+	if t.buf.Len() != 0 {
+		tokenLen += len(t.delim)
+	}
+	if t.lineLen < t.buf.Len()+tokenLen {
+		t.result = append(t.result, t.buf.String())
+		t.buf.Reset()
+	}
+	if t.buf.Len() != 0 {
+		t.buf.WriteString(t.delim)
+	}
+	for _, part := range parts {
+		t.buf.WriteString(part)
+	}
+}
+
 // Lines terminates the line-building and returns all the lines.
 func (t *TokenLineBuilder) Lines() (result []string) {
 	result = t.result

irc/utils/text_test.go

@@ -43,3 +43,26 @@ func TestBuildTokenLines(t *testing.T) {
 	val = BuildTokenLines(10, []string{"abcd", "efgh", "ijkl"}, ",")
 	assertEqual(val, []string{"abcd,efgh", "ijkl"}, t)
 }
+
+func TestTLBuilderAddParts(t *testing.T) {
+	var tl TokenLineBuilder
+	tl.Initialize(20, " ")
+	tl.Add("bob")
+	tl.AddParts("@", "alice")
+	tl.AddParts("@", "ErgoBot__")
+	assertEqual(tl.Lines(), []string{"bob @alice", "@ErgoBot__"}, t)
+}
+
+func BenchmarkTokenLines(b *testing.B) {
+	tokens := strings.Fields(monteCristo)
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		var tl TokenLineBuilder
+		tl.Initialize(400, " ")
+		for _, tok := range tokens {
+			tl.Add(tok)
+		}
+		tl.Lines()
+	}
+}