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

Remove redundant ownership check #158

Merged

Conversation

ypconstante
Copy link
Contributor

Today Mox does a ownership check before doing an update to store the expectations. This call can be avoided because NimbleOwnership already does this same check on get_and_update, and the error allows us to do the error mapping.

Name                   ips        average  deviation         median         99th %
bench (PR)         36.47 K       27.42 μs    ±34.15%       25.69 μs       61.83 μs
bench (main)       32.33 K       30.93 μs    ±34.77%       28.91 μs       66.20 μs

Comparison: 
bench (PR)         36.47 K
bench (main)       32.33 K - 1.13x slower +3.52 μs

Memory usage statistics:

Name                 average  deviation         median         99th %
bench (PR)           7.08 KB     ±0.02%        7.08 KB        7.08 KB
bench (main)         7.53 KB     ±0.15%        7.53 KB        7.53 KB
defmodule BenchBehaviour do
  @functions Enum.map(1..50, &:"get_v#{&1}")

  for function <- @functions do
    @callback unquote(function)() :: integer()
  end
end

defmodule BenchBehaviourImpl do
  @behaviour BenchBehaviour

  @functions Enum.map(1..50, &:"get_v#{&1}")

  for function <- @functions do
    @impl BenchBehaviour
    def unquote(function)() do
      unquote(function)
    end
  end
end

Mox.defmock(BenchBehaviourMock, for: BenchBehaviour)

Benchee.run(
  %{
    "bench" => fn ->
      Mox.stub_with(BenchBehaviourMock, BenchBehaviourImpl)
      NimbleOwnership.cleanup_owner({:global, Mox.Server}, self())
    end
  },
  time: 10,
  memory_time: 2
)

@josevalim
Copy link
Member

/cc @whatyouhide

Copy link
Collaborator

@whatyouhide whatyouhide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only left one comment around returning errors, but this is a great catch yep! I also pushed dashbitco/nimble_ownership@57e5513 to improve the docs for get_and_update/5 and make this case hopefully clearer.

lib/mox.ex Outdated Show resolved Hide resolved
@ypconstante ypconstante force-pushed the remove-redundant-ownership-check branch from f93cd2f to 79a2363 Compare June 23, 2024 13:38
@josevalim josevalim merged commit 26dd4cb into dashbitco:main Jun 25, 2024
2 checks passed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@ypconstante ypconstante deleted the remove-redundant-ownership-check branch June 25, 2024 16:17
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.

3 participants