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

Replace Code.ensure_loaded/1 with faster counterpart #52

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timbuchwaldt
Copy link

Our benchmarks showed that Code.ensure_loaded/1 becomes the bottleneck of our system when validating data with Vex.

I built a cached version of Code.ensure_loaded/1 that utilises FastGlobal to store a list of modules loaded in such a way.

This change brought a ~10x performance improvement on a real world application.

@benwilson512
Copy link
Member

It's been a very long time since I've looked at Vex code but yeah there definitely shouldn't be any Code.ensure_loaded calls at runtime. A faster / cached version is nice, but I think it makes more sense to try to design this such that the call doesn't happen at runtime at all.

@timbuchwaldt
Copy link
Author

Hey Ben. True that, not a beautiful solution, especially since in a normal production system Code.ensure_loaded/1 is not needed, as all code is loaded anyway.

We contemplated making Vex an application that is actually startable instead of a plain library, then ensuring loading all validators during startup - how does that sound to you?

@benwilson512
Copy link
Member

What are the validators? Why might they not be loaded?

@benwilson512
Copy link
Member

The normal use for Code.ensure_loaded? is something like

if Code.ensure_loaded?(Ecto) do
  def list_foos() do
    # if ecto is also around, make this function do stuff
  end
else
  def list_foos, do: raise "only available if ecto is also a dependency"
end

This is a common pattern if you have library A that has an optional dependency on library B. For people who have B also installed, they get certain functions in A. All of this happens at compile time though, so no runtime loading is necessary either at application start or any other time.

@timbuchwaldt
Copy link
Author

The problem is the usage of function_exported?/3. It returns false if the module isn't currently loaded, which can happen in dev/test if it hasn't been called before.

Thinking about it the implementation kind of looks like a Protocol in disguise, but have to look a bit deeper.

@benwilson512
Copy link
Member

Yeah I was getting the same "this looks like protocols" vibe. The original commits for the ensure loaded lines come from when Elixir would have been ~v0.10 so things have come a long way since then hahaha.

@timbuchwaldt
Copy link
Author

Damn - and this is how a quick fix turns into a quest of refactoring :D

@benwilson512
Copy link
Member

In looking at this more closely, the ensure loaded stuff may be completely unnecessary. I think it hails from a time when protocol implementation loading worked differently.

Can you try a branch where you just simply remove the Code.ensure_loaded calls? Do you run into any issues?

@uberbrodt
Copy link

I spent some time looking at this, and the Code.ensure_loaded/1 calls are still necessary when the VM isn't running in embedded mode. I'm not sure there's a way to fix it that preserves the existing API. Turning it into an OTP application wouldn't really help us, because Vex constructs module names at runtime based on the name used to call a validator.

I think that requiring users to load the validators in their Application callback is the least "tricky" way of removing the calls to Code.ensure_loaded/1. Something like Vex.sources/1 or Vex.set_validators/1, where you pass in a list of validator modules.

@jfornoff
Copy link
Collaborator

jfornoff commented Jun 18, 2018 via email

@uberbrodt
Copy link

uberbrodt commented Jun 18, 2018 via email

@jfornoff
Copy link
Collaborator

jfornoff commented Jun 18, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants