Epic: Make String Builder thread safe

[varunu28]

This is a multi-step effort to make string builder thread-safe and allow users to use it in a multi-threaded environment.

Keeping this open for discussion and narrow down on what particular tasks need be accomplished in order to convert the current implementation to a thread-safe implementation.

[varunu28]

cc @linkdotnet

[linkdotnet]

We could introduce a mutexlike this:

type StringBuilder struct {
    data     []rune
    position int
    mu       sync.Mutex
}

And then throughout the code use the mutex

func (s *StringBuilder) Append(text string) *StringBuilder {
    s.mu.Lock() 
    defer s.mu.Unlock()
    
    s.resize(text)
    textRunes := []rune(text)
    copy(s.data[s.position:], textRunes)
    s.position = s.position + len(textRunes)
    
    return s
}

Under normal circumstances this should be cheap (given single threaded access). My fear is that in tight loops it drops the performance quite a lot.

Option 1

Two implementations. One thread safe and one non-thread safe. Like this:

type StringBuilder interface {
    Append(string) *StringBuilder
    // ... other methods ...
}

type ThreadSafeStringBuilder struct {
    mu sync.Mutex
    // ... other fields ...
}

func (s *ThreadSafeStringBuilder) Append(text string) *ThreadSafeStringBuilder {
    s.mu.Lock()
    defer s.mu.Unlock()
    // ... actual implementation ...
    return s
}

type NonThreadSafeStringBuilder struct {
    // ... fields ...
}

func (s *NonThreadSafeStringBuilder) Append(text string) *NonThreadSafeStringBuilder {
    // ... actual implementation ...
    return s
}

Option 2 (currently my preferred one)

Make the current one have a flag for the mutex:

type StringBuilder struct {
    mu            sync.Mutex
    useMutex      bool
    // ... other fields ...
}

func NewStringBuilder(useMutex bool) *StringBuilder {
    return &StringBuilder{useMutex: useMutex}
}

func (s *StringBuilder) Append(text string) *StringBuilder {
    if s.useMutex {
        s.mu.Lock()
        defer s.mu.Unlock()
    }
   ...

But that would duplicate almost all of our c'tors.

Any other ideas @varunu28

[varunu28]

We could introduce a mutexlike this:

type StringBuilder struct {
    data     []rune
    position int
    mu       sync.Mutex
}

And then throughout the code use the mutex

func (s *StringBuilder) Append(text string) *StringBuilder {
    s.mu.Lock() 
    defer s.mu.Unlock()
    
    s.resize(text)
    textRunes := []rune(text)
    copy(s.data[s.position:], textRunes)
    s.position = s.position + len(textRunes)
    
    return s
}

Under normal circumstances this should be cheap (given single threaded access). My fear is that in tight loops it drops the performance quite a lot.

Option 1

Two implementations. One thread safe and one non-thread safe. Like this:

type StringBuilder interface {
    Append(string) *StringBuilder
    // ... other methods ...
}

type ThreadSafeStringBuilder struct {
    mu sync.Mutex
    // ... other fields ...
}

func (s *ThreadSafeStringBuilder) Append(text string) *ThreadSafeStringBuilder {
    s.mu.Lock()
    defer s.mu.Unlock()
    // ... actual implementation ...
    return s
}

type NonThreadSafeStringBuilder struct {
    // ... fields ...
}

func (s *NonThreadSafeStringBuilder) Append(text string) *NonThreadSafeStringBuilder {
    // ... actual implementation ...
    return s
}

Option 2 (currently my preferred one)

Make the current one have a flag for the mutex:

type StringBuilder struct {
    mu            sync.Mutex
    useMutex      bool
    // ... other fields ...
}

func NewStringBuilder(useMutex bool) *StringBuilder {
    return &StringBuilder{useMutex: useMutex}
}

func (s *StringBuilder) Append(text string) *StringBuilder {
    if s.useMutex {
        s.mu.Lock()
        defer s.mu.Unlock()
    }
   ...

But that would duplicate almost all of our c'tors.

Any other ideas @varunu28

Option 2 makes sense as we avoid duplicating the code and also end up making a decision that going forward all the methods of StringBuilder will be thread-safe.

Also we will need to change the contract(Or working) of AsRuneArray(And similar) functions to make sure the implementation remains thread-safe as we discussed earlier.

Side question: I am not so aware of how package versioning works? If I work on an implementation, will I have to do something different in order to make sure the new features land on version 1?

[linkdotnet]

Do we have change the contract to make it work? A shared mutex doesn’t do the trick?

basically, I have a GitHub action that created the package and the version is the input. So we either could create a v1 branch and merge or once we feel confident (into main), or just push everything to main and when we are done we release it. The latter option is leaner but makes other fixed difficult (for the version 0 stuff).

As we don’t have a specific roadmap (except the two open items) we can just merge everything to v1 and merge that once we feel ready

[linkdotnet]

That said I am not sure if it is worth slicing the task into smaller sub tasks or just make one pass through and use the mutex everywhere.

[varunu28]

Do we have change the contract to make it work?

Not necessarily. Though as we are currently planning a move to v1, we can rethink any contract changes that we want to introduce as it is initial phase of the version.

@linkdotnet Makes sense. I will create a v1 branch and try to take a shot at it in one go.

[linkdotnet]

Fyi I created v1 - it's not only the branch but the actions should be adopted as well.

[varunu28]

Resolved by #14