Deprecating things for better performance

[okuramasafumi]

Opening this as a draft because as noted above and in code comments, there are a number of Alba feature / flexibility that conflict a bit with this approach. So there would be some extra work needed to not break compatibility in some corner cases.

From #424

I'd like to discuss performance and deprecation further here.
My personal idea is to add Alba.freeze so that we can make sure every config is static. Typically it would be called in the initializer.

@byroot What do you think? Do you have any method you want to deprecate?

[byroot]

My personal idea is to add Alba.freeze so that we can make sure every config is static.

Interesting. I suppose that would solve the issue with needing to clear the attributes caches when Alba.reset_transform_keys is called. We could instead not cache until Alba.freeze is called.

The downside I think is many users might not notice this is needed, and it's not easy to test such behavior, as you can only freeze Alba once.

An alternative could be to have something like Alba.serial # => 0. Whenever you change something that should clear the caches, we bump that integer. Then resources can check if the "version" was bumped, makes for an extra variable access and compare.

Yet another possibility is to continue doing something similar to ObjectSpace.each_object but use a WeakMap to keep a weak reference to all Resource classes.

[byroot]

think using ObjectSpace.each_object is a bit too much though.

Definitely. That last suggestion wouldn't use it anymore.

[okuramasafumi]

@byroot With serial strategy, will we have version numbers for each resource, or for a whole process?

[byroot]

will we have version numbers for each resource, or for a whole process?

Kinda both?

The idea would be to have Alba._serial # => 42.

Then resources would be able to do thing like:

def attributes_hash
  if @_serial != Alba._serial
    recomputed_cached_state
  end
end

It's really not my favorite solution though.

[okuramasafumi]

@byroot Thank you.
I think I'd like to go with Alba.freeze, since it's simpler and practical enough (after initializer, most users don't want to change their configurations).
The problem is how to let users call Alba.freeze. This is not mandatory but rather performance bonus, so maybe we can just add CHANGELOG entry for this change. The more aggressive way is to print warnings when it's not freezed, but I think it's a little too much.

[okuramasafumi]

What about introducing something like Alba.final and Alba.final?. This is just a declaration instead of actually freezing it. We can trust users that they don't change anything affecting internal cache after they call Alba.final. This is easier to test since Alba.final is just an attribute of Alba module.
The remaining problem is we cannot force users to call it. I came up with the idea to put "finalizing" logic in railtie, but for some users this might cause a weird bug. I'm still considering...

[byroot]

Other methods that aren't ideal with the approach I took in #424

• def select: it can be supported, I use a slowpath for it.
• def attributes: currently have to de-optimize when it's redefined.

Both are ways to not include one of the attributes in the result, which can also be done via returning Alba::REMOVE_KEY, or declaring the attribute with an :if condition. That's 4 different ways to do the same thing. I understand that it makes Alba very flexible, but that makes optimization harder.

But to be clear, I don't want to encourage you to deprecate anything. I'm just exploring how Alba could be made faster, it doesn't have to be merged if you'd rather keep this flexibility in the gem.

[okuramasafumi]

The reason behind select is to make it possible to filter with "values". It's basically a superset of attributes instance method, so I think deprecating attributes is fine.

[okuramasafumi]

@byroot I think now I understand it, so from attribute or association, we can return Alba::REMOVE_KEY to remove it from the result.

class FooResource
  include Alba::Resource

  attribute :bar do
    b = object.bar
    b.nil? ? Alba::REMOVE_KEY : b
  end
end

I know it's good for optimization, but what about attributes? With select, we can skip it from the result when the value of the attribute is nil. With attributes there's not way to return explicit values, so returning Alba::REMOVE_KEY is not possible.

[byroot]

You mean something like:

def attributes
  super - [:email]
end

If so, then yes, it's hard to optimize that feature. It can be handled like in my current proof of concept, by deoptimizing when we notice the feature is used.

I guess where I'm getting at, is having multiple ways to do the same thing necessarily impose an overhead.

But I'd like to re-iterate that I'm not asking you to make any particular change, I was just curious about how far I could push Alba if I allowed myself to remove some features.

[okuramasafumi]

@byroot

But I'd like to re-iterate that I'm not asking you to make any particular change, I was just curious about how far I could push Alba if I allowed myself to remove some features.

Yes, the most important part of Alba is usability, so I won't sacrifice usability for performance reasons.
However, when there are multiple ways to do the same thing, it doesn't necessarily mean it's more usable. In fact, attributes instance method was not designed well, and after I came up with select it existed basically just for backward compatibility. So deprecating it is no-brainer.

Now it's good time to release the next major version, v4, since there are other deprecations.
My plan is to soon release a next minor version, 3.6.0, with more deprecations, and then release a new major version, 4.0.0, hopefully in April.
So if you have an idea of deprecating some methods for better performance, not only attributes, just let me know!

[okuramasafumi]

I tried to deprecate attributes and make it print a warning when overridden. It turned out it's not so easy, since we also need to change select method signature. It cannot be done at the same time without printing lots of false warnings.
I'm planning to soft-deprecate attributes method in 4.0.0, with select taking additional arguments so that select can be alternative to attributes. With version 5 we can completely remove attributes call and safely optimize it.

[okuramasafumi]

@trevorturk Hi Trevor, sorry that I forgot to mention you in this discussion. If you have some ideas, please make a post here!