Change immutable class to struct

[j8r]

I noticed, after looking at the implementations, some places where classes can be changed to structs, because the objects are immutable.

[wooster0]

Can't Levenshtein::Finder be a struct too?

[j8r]

@r00ster91 not sure, because of @best_entry. We have to check if it is passed by reference.

[jkthorne]

Is this for performance?
If so have you benchmarked this?

[wooster0]

Yes, this is for performance, see: https://crystal-lang.org/reference/guides/performance.html#use-structs-when-possible.

@j8r I think it would work to make it a struct. @best_entry is of type Entry which is a struct too. And in my tests it all worked as expected: https://carc.in/#/r/7o5s.

[jkthorne]

I thought it might be. I was just hoping to verify there was a performance benefit if that is what the change is for.

[asterite]

Yes, please provide benchmarks. Group is kind of huge, maybe copying that is slow.

[j8r]

I can provide benchmarks, there is likely little difference. It's not only about performance, but also to mark the immutability of the object.

Usually, I'm more confident when I see structs because there is little to no side effects involved, with often better performance/memory usage of course.

[wooster0]

Hmm what about String? In the description it even says:

A String represents an immutable sequence of UTF-8 characters.

[j8r]

@r00ster91 String is a false immutable. It is immutable when using only safe methods, but can be mutated with to_unsafe.

[j8r]

Benchmark:

def method(s)
  s
end

Benchmark.ips do |x|
  x.report("Group class") do
    g = System::Group.find_by name: "root"
    method g
  end

  x.report("Group struct") do
    g = System::GroupStruct.find_by name: "root"
    method g
  end
end

Benchmark.ips do |x|
  x.report("User class") do
    u = System::User.find_by name: "root"
    method u
  end

  x.report("User struct") do
    u = System::UserStruct.find_by name: "root"
    method u
  end
end

Result:

 Group class 231.27k (  4.32µs) (± 1.52%)  64.0B/op        fastest
Group struct 229.08k (  4.37µs) (± 3.56%)  32.0B/op   1.01× slower
 User class 208.26k (  4.80µs) (± 8.51%)  288B/op   1.03× slower
User struct 214.88k (  4.65µs) (± 5.18%)  224B/op        fastest

As expected, not much difference.

[jkthorne]

Group Struct is slower?

[wooster0]

1.01× slower usually means nothing and is just a coincidence. Often if you re-run the benchmark the other one is suddenly 1.01× slower.

[ysbaddaden]

Being immutable isnt the only matter. Transforming a small class into a struct can improve performance, but a large class with many ivars can involve lots of memcpy instead of just passing a pointer.

Your benchmark doesnt test copying because the method call will be inlined by LLVM.

Making Process::Status a struct makes perfect sense: it merely wraps a 32-bit integer, thus will be pased inside a register (no memcpy) and it's immutable.

Making System::User doesnt make sense: 6 string structs, most nilable. The class is huge. Passing it by value will require lots of memcpy.

[j8r]

I have to add @[NoInline] then, I guess.
A String struct? String is a class, so even if the object is a struct, the ivars would be passed as references (like Array, Hash, etc).

[jkthorne]

I understand performance is not the only reason why to make this change. I just thought it would be the way.

[j8r]

Updated benchmark:

@[NoInline]
def another(s)
  s
end

@[NoInline]
def method(s)
  999.times { another s }
end

Results:

 Group class 212.98k (  4.70µs) (± 8.67%)  64.0B/op   1.04× slower
Group struct 222.26k (  4.50µs) (± 3.49%)  32.0B/op        fastest
 User class 208.78k (  4.79µs) (± 4.33%)  288B/op   1.01× slower
User struct 211.63k (  4.73µs) (± 3.65%)  224B/op        fastest

[j8r]

As I expected @r00ster91 , changing Levenshtein::Finder to a struct make the CI failing.

[bcardiff]

changing Process::Status seems fine
changing OAuth::RequestToken is unlikely to have any effect, is not a type that is used a lot I think.

But I would not change Group or User as expressed by others.

[j8r]

As measured @bcardiff , there is no difference.
Please prove me wrong, I may have done an incorrect benchmark.

[j8r]

Any update?
As said previously, I have verified that changing the User and Group classes to structs doesn't degrade the performance.
If you still prefer, I can drop this changes.

[j8r]

Is it possible to move forward? I noticed other objects that can be changed from class to struct.

HTTP::Server, which is mainly immutable, except for HTTP::Server#bind, which adds an address to the @sockets array, which is a class passed by reference - so we are good.

[asterite]

Sorry, but this is not needed. This will cause breaking changes without real benefits.

[j8r]

For HTTP::Server, or others? For HTTP::Server, why not, because it is not expected to create a lot and pass them around.
For other objects, it can be beneficial.

[asterite]

For HTTP::Server I can see there's a property error_io so it's already not immutable.

But generally, how many http servers are created per app? One? It doesn't make a difference if it's a class or a struct. And a class is more versatile because you can inherit from it.

[j8r]

Yes I agree for HTTP::Server, even if it is strange to choose class for the only benefit of inheriting from it.

[j8r]

And property is only for testing, it is not supposed to be mutable otherwise. I drop the commit

[straight-shoota]

The default for every instance type in Crystal API's should be class because it's more versatile and easier to use. General exceptions are only types for simple immutable value objects (typically using the record macro).
struct can also be considered when there's a definitive performance concern.

But just refactoring classes to structs because they're immutable types doesn't make sense. Especially when they're not inherently immutable but just happen to have no mutable state in the current implementation. Classes can be immutable. That's perfectly fine and there's no inherent benefit to converting them to structs.

[straight-shoota]

OAuth::RequestToken and Process::Status are types for value objects. IMO they should be refactored to structs.
Everything else should be left as is.

[j8r]

Here OAuth and Process modifications make sense, IMO. They can have an impact, since an application could easily use them a lot.

I don't know what do you mean by "inherently immutable", there is no golden rule for that. There are obvious simp'e examples, but we can just look at the implementation and the public API to know if an object is mutable or not – and switching between class and struct will be transparent (except for inheritance).

The main questions are: is it mutable, and is inheritance needed?

[j8r]

Why not for the other OAuth classes?

[straight-shoota]

I don't know what do you mean by "inherently immutable", there is no golden rule for that.

By inherently immutable I mean value objects and similar concepts.

we can just look at the implementation and the public API to know if an object is mutable or not

This only covers the current implementation and API. Later changes adding mutable state would mean a break change from struct to class. And so does the current change from class to struct. Since there's no real benefit from doing that in the first place, we shouldn't even bother having this discussion.

and switching between class and struct will be transparent (except for inheritance).

It's not transparent but a breaking change affecting inheritance, reopening and instance behaviour.

Why not for the other OAuth classes?

Because they're behavioural models, not value objects.

[j8r]

I guess I can use memoization and a hash table to avoid creating a new OAuth2 client for each request.

[yxhuvud]

Extracting Oauth connections to a pool or whatnot may very well make sense from an efficiency standpoint (new connections are not free), but is this PR really a good place to do that kind of change? Wouldn't it make more sense to drive that kind of change because it is good and necessary rather than doing it in some quest for purity?

[j8r]

I was only talking about my use case, on an efficiency point a view – not changes to make here.

[j8r]

In fact, in my app, a proxy, a lot of requests are processed – I was investigating ways to improve processing time.
For instance, I created an HTTP::Client before hand instead of using HTTP::Client.get for each request.

[j8r]

(My 3 last comments are not related to this PR, I explain the rationale from my point a view for those wondering why)

[Sija]

@j8r Please, change this PR title and description accordingly to your latests changes.

[straight-shoota]

Just for the sake of argument: OAuth2::Client is currently super inefficient when it comes to high volumes because every single request initializes a new HTTP::Client and thus a new TCP connection. But that needs to be fixed in HTTP::Client to reuse existing connections.

[j8r]

Is to OK to merge now? Only OAuth::RequestToken and Process::Status are changed to structs at the end.