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

Liveview cohabitation #192

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

Conversation

guidotripaldi
Copy link
Contributor

@guidotripaldi guidotripaldi commented Aug 14, 2019

Currently it is not possible to use both Drab and LiveView in the same application because Drab.Client.generate_drab_js/3 rises when it tries to get the controller module for a LiveView page, since a :phoenix_controller is not present in the conn of a page rendered by LiveView.

Namely, we have to check against the availability of a Controller before to try to access it instead of blindly assume that it is always available, as it was true in the world before the advent of LV.

This patch introduces two new helpers, get_controller_module/1 and get_controller_action_name/1, that let Drab.Client.generate_drab_js/3 to conditionally access the Controller data, and thus generating the Drab js, only when the Controller is actually available.

@spec get_controller_module(Plug.Conn.t()) :: atom() | nil
defp get_controller_module(conn) do
try do
Phoenix.Controller.controller_module(conn)
Copy link
Collaborator

Choose a reason for hiding this comment

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

For both this and the one in get_controller_action_name is there no way to more safely check this without incurring an exception? Exceptions are pretty cheap when not thrown (although setting up the exception handler is not free in any case), but they are much less cheap when they are actually thrown, which would be very common in LiveView cases. If not this looks good as it is. :-)

Copy link
Contributor Author

@guidotripaldi guidotripaldi Aug 14, 2019

Choose a reason for hiding this comment

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

I don't like it either use exceptions in a library but I haven't found a better way, both Phoenix.Controller.controller_module/1 and Phoenix.Controller.action_name/1 raise if the Controller is unavailable.

Any suggestions for some alternatives of course are welcome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, all it checks is conn.private[:phoenix_controller], similar for the other, which is technically internal and not exposed, although it's never been changed. I wonder if direct access might be warranted, thoughts?

Copy link
Contributor Author

@guidotripaldi guidotripaldi Aug 14, 2019

Choose a reason for hiding this comment

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

In fact my first version was similar, it was a check against the presence of : live_view_module in the assigns ( if !conn.assigns[:live_view_module] do), but it was a solution too specific for LV, so searching for a a general solution I thought that is better to use the official API because, although unlikely, the parameters for a direct access could be changed tomorrow, and in a library maybe is preferable do not hack too much.

Copy link
Contributor Author

@guidotripaldi guidotripaldi Aug 14, 2019

Choose a reason for hiding this comment

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

Actually I don't know how much heavy thrown exceptions are, it could be interesting to setup some benchmark so to be aware of the actual costs. In any case we could assume that we don't want exceptions management in Drab and use the conn.private[:phoenix_controller] hack, and switch to the API based solution if tomorrow things will change.

Copy link
Collaborator

@OvermindDL1 OvermindDL1 Aug 14, 2019

Choose a reason for hiding this comment

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

Hmm, let's test this case:

╰─➤  mix bench map_lookup_or_throw                                                                                                1 ↵
Operating System: Linux"
CPU Information: AMD Phenom(tm) II X6 1090T Processor
Number of Available Cores: 6
Available memory: 15.67 GB
Elixir 1.8.1
Erlang 21.2.2

Benchmark suite executing with the following configuration:
warmup: 2 s
time: 2 s
memory time: 2 s
parallel: 1
inputs: large-10000-existing, large-9999-not-existing, medium-10-existing, medium-9-not-existing, small-1-not-existing, small-2-existing
Estimated total run time: 3 min


Benchmarking Access with input large-10000-existing...
Benchmarking Access with input large-9999-not-existing...
Benchmarking Access with input medium-10-existing...
Benchmarking Access with input medium-9-not-existing...
Benchmarking Access with input small-1-not-existing...
Benchmarking Access with input small-2-existing...
Benchmarking Map.fetch with input large-10000-existing...
Benchmarking Map.fetch with input large-9999-not-existing...
Benchmarking Map.fetch with input medium-10-existing...
Benchmarking Map.fetch with input medium-9-not-existing...
Benchmarking Map.fetch with input small-1-not-existing...
Benchmarking Map.fetch with input small-2-existing...
Benchmarking case with input large-10000-existing...
Benchmarking case with input large-9999-not-existing...
Benchmarking case with input medium-10-existing...
Benchmarking case with input medium-9-not-existing...
Benchmarking case with input small-1-not-existing...
Benchmarking case with input small-2-existing...
Benchmarking catch-Map.fetch! with input large-10000-existing...
Benchmarking catch-Map.fetch! with input large-9999-not-existing...
Benchmarking catch-Map.fetch! with input medium-10-existing...
Benchmarking catch-Map.fetch! with input medium-9-not-existing...
Benchmarking catch-Map.fetch! with input small-1-not-existing...
Benchmarking catch-Map.fetch! with input small-2-existing...
Benchmarking catch-direct-always-fail with input large-10000-existing...
Benchmarking catch-direct-always-fail with input large-9999-not-existing...
Benchmarking catch-direct-always-fail with input medium-10-existing...
Benchmarking catch-direct-always-fail with input medium-9-not-existing...
Benchmarking catch-direct-always-fail with input small-1-not-existing...
Benchmarking catch-direct-always-fail with input small-2-existing...

##### With input large-10000-existing #####
Name                               ips        average  deviation         median         99th %
case                           11.18 M      0.0895 μs     ±6.43%      0.0880 μs       0.106 μs
catch-Map.fetch!                9.60 M       0.104 μs   ±202.32%       0.100 μs       0.150 μs
Map.fetch                       9.54 M       0.105 μs   ±211.47%       0.100 μs        0.22 μs
Access                          7.39 M       0.135 μs    ±17.79%       0.130 μs        0.20 μs
catch-direct-always-fail        5.90 M       0.169 μs    ±56.58%       0.160 μs        0.25 μs

Comparison: 
case                           11.18 M
catch-Map.fetch!                9.60 M - 1.16x slower
Map.fetch                       9.54 M - 1.17x slower
Access                          7.39 M - 1.51x slower
catch-direct-always-fail        5.90 M - 1.89x slower

Memory usage statistics:

Name                        Memory usage
case                           529.25 KB
catch-Map.fetch!               529.25 KB - 1.00x memory usage
Map.fetch                      529.27 KB - 1.00x memory usage
Access                         529.25 KB - 1.00x memory usage
catch-direct-always-fail       529.28 KB - 1.00x memory usage

**All measurements for memory usage were the same**

##### With input large-9999-not-existing #####
Name                               ips        average  deviation         median         99th %
case                           13.22 M      0.0756 μs    ±15.54%      0.0700 μs       0.120 μs
Map.fetch                      10.46 M      0.0956 μs    ±16.55%      0.0900 μs       0.140 μs
Access                          8.05 M       0.124 μs    ±17.15%       0.120 μs       0.180 μs
catch-Map.fetch!                5.75 M       0.174 μs   ±228.92%       0.160 μs        0.27 μs
catch-direct-always-fail        5.69 M       0.176 μs   ±276.39%       0.160 μs        0.37 μs

Comparison: 
case                           13.22 M
Map.fetch                      10.46 M - 1.26x slower
Access                          8.05 M - 1.64x slower
catch-Map.fetch!                5.75 M - 2.30x slower
catch-direct-always-fail        5.69 M - 2.32x slower

Memory usage statistics:

Name                        Memory usage
case                           529.20 KB
Map.fetch                      529.20 KB - 1.00x memory usage
Access                         529.20 KB - 1.00x memory usage
catch-Map.fetch!                0.180 KB - 0.00x memory usage
catch-direct-always-fail       529.23 KB - 1.00x memory usage

**All measurements for memory usage were the same**


##### With input medium-10-existing #####
Name                               ips        average  deviation         median         99th %
case                           16.57 M      0.0604 μs     ±6.70%      0.0590 μs      0.0733 μs
catch-Map.fetch!               11.85 M      0.0844 μs     ±6.72%      0.0820 μs       0.105 μs
Map.fetch                      11.00 M      0.0909 μs   ±434.54%      0.0800 μs        0.21 μs
Access                         10.08 M      0.0992 μs     ±7.51%      0.0960 μs       0.124 μs
catch-direct-always-fail        6.17 M       0.162 μs   ±232.69%       0.150 μs        0.32 μs

Comparison: 
case                           16.57 M
catch-Map.fetch!               11.85 M - 1.40x slower
Map.fetch                      11.00 M - 1.51x slower
Access                         10.08 M - 1.64x slower
catch-direct-always-fail        6.17 M - 2.69x slower

Memory usage statistics:

Name                        Memory usage
case                               592 B
catch-Map.fetch!                   592 B - 1.00x memory usage
Map.fetch                          616 B - 1.04x memory usage
Access                             592 B - 1.00x memory usage
catch-direct-always-fail           624 B - 1.05x memory usage

**All measurements for memory usage were the same**

##### With input medium-9-not-existing #####
Name                               ips        average  deviation         median         99th %
case                           18.67 M      0.0536 μs    ±19.03%      0.0500 μs      0.0800 μs
Map.fetch                      12.84 M      0.0779 μs     ±6.68%      0.0760 μs      0.0970 μs
Access                         10.50 M      0.0952 μs    ±27.39%      0.0900 μs       0.140 μs
catch-Map.fetch!                6.55 M       0.153 μs    ±77.30%       0.140 μs        0.25 μs
catch-direct-always-fail        6.27 M       0.159 μs   ±207.46%       0.150 μs        0.23 μs

Comparison: 
case                           18.67 M
Map.fetch                      12.84 M - 1.45x slower
Access                         10.50 M - 1.78x slower
catch-Map.fetch!                6.55 M - 2.85x slower
catch-direct-always-fail        6.27 M - 2.98x slower

Memory usage statistics:

Name                        Memory usage
case                               552 B
Map.fetch                          552 B - 1.00x memory usage
Access                             552 B - 1.00x memory usage
catch-Map.fetch!                   184 B - 0.33x memory usage
catch-direct-always-fail           584 B - 1.06x memory usage

**All measurements for memory usage were the same**

##### With input small-1-not-existing #####
Name                               ips        average  deviation         median         99th %
case                           16.93 M      0.0591 μs     ±8.03%      0.0590 μs      0.0830 μs
Map.fetch                      14.18 M      0.0705 μs    ±41.16%      0.0680 μs      0.0890 μs
Access                         11.45 M      0.0874 μs    ±26.97%      0.0800 μs       0.130 μs
catch-Map.fetch!                6.78 M       0.148 μs   ±182.52%       0.140 μs        0.26 μs
catch-direct-always-fail        6.20 M       0.161 μs   ±288.91%       0.150 μs        0.26 μs

Comparison: 
case                           16.93 M
Map.fetch                      14.18 M - 1.19x slower
Access                         11.45 M - 1.48x slower
catch-Map.fetch!                6.78 M - 2.50x slower
catch-direct-always-fail        6.20 M - 2.73x slower

Memory usage statistics:

Name                        Memory usage
case                               232 B
Map.fetch                          232 B - 1.00x memory usage
Access                             232 B - 1.00x memory usage
catch-Map.fetch!                   184 B - 0.79x memory usage
catch-direct-always-fail           264 B - 1.14x memory usage

**All measurements for memory usage were the same**

##### With input small-2-existing #####
Name                               ips        average  deviation         median         99th %
case                           17.26 M      0.0579 μs    ±48.24%      0.0580 μs      0.0810 μs
catch-Map.fetch!               13.85 M      0.0722 μs    ±55.45%      0.0700 μs       0.100 μs
Map.fetch                      13.08 M      0.0765 μs     ±9.96%      0.0720 μs       0.109 μs
Access                         11.07 M      0.0903 μs     ±8.15%      0.0890 μs       0.110 μs
catch-direct-always-fail        6.18 M       0.162 μs   ±220.42%       0.150 μs        0.35 μs

Comparison: 
case                           17.26 M
catch-Map.fetch!               13.85 M - 1.25x slower
Map.fetch                      13.08 M - 1.32x slower
Access                         11.07 M - 1.56x slower
catch-direct-always-fail        6.18 M - 2.79x slower

Memory usage statistics:

Name                        Memory usage
case                               272 B
catch-Map.fetch!                   272 B - 1.00x memory usage
Map.fetch                          296 B - 1.09x memory usage
Access                             272 B - 1.00x memory usage
catch-direct-always-fail           304 B - 1.12x memory usage

**All measurements for memory usage were the same**

So in summary, matching via a case extraction is always fastest. The catch-direct-always-fail will be the case where Phoenix's functions will lookup and fail (and it always fails for the same reason that phoenix hard-codes in its keys). The Access pattern is the some_map[some_key] lookup, it's not great either.

In short, a case matcher would be about triple the speed on average. I'm not sure that's really worth caring about then. I'm voting to accept it with the Phoenix calls as it is then. Agree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the benchmark, interesting results.
I agree, differences are not substantial, and in case we can change our mind anytime

Copy link
Collaborator

@OvermindDL1 OvermindDL1 left a comment

Choose a reason for hiding this comment

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

Some removals

lib/drab/client.ex Outdated Show resolved Hide resolved
lib/drab/client.ex Outdated Show resolved Hide resolved
@OvermindDL1
Copy link
Collaborator

I'll leave this for a day for anyone else to look over as I have not ran LiveView and Drab at the same time, remind me if I miss it. ^.^

It would be nice to get a test case for this with LiveView though!

@guidotripaldi
Copy link
Contributor Author

guidotripaldi commented Aug 15, 2019

It would be nice to get a test case for this with LiveView though!

test added.

@OvermindDL1
Copy link
Collaborator

OvermindDL1 commented Aug 15, 2019

Those look really good, I'll go over them the next time I get a minute, probably tomorrow, thanks! :-)

@OvermindDL1
Copy link
Collaborator

OvermindDL1 commented Aug 16, 2019

Looks great, however some tests aren't passing, I'm looking over them currently... Test seed: 509794

I don't think this one is related:

15:35:14.711 [warn] element click intercepted: Element <button id="_drab_modal_button_addtional" name="additional" type="button" class="btn btn-default drab-modal-button" data-dismiss="modal">...</button> is not clickable at point (647, 210). Other element would receive the click: <button name="ok" type="submit" class="btn btn-primary drab-modal-button" data-dismiss="modal">...</button>                                                                                             
  (Session info: headless chrome=76.0.3809.100)                                                                                       
  (Driver info: chromedriver=76.0.3809.100 (ed9d447d30203dc5069e540f05079e493fc1c132-refs/branch-heads/3809@{#990}),platform=Linux 4.4.0-140-generic x86_64)                                                                                                                


  1) test Drab.Modal modal over modal (DrabTestApp.ModalTest)
     test/integration/modal_test.exs:92
     Assertion with == failed
     code:  assert visible_text(find_element(:id, "modal5_out")) == "{:additional, %{}}"
     left:  ""
     right: "{:additional, %{}}"
     stacktrace:
       test/integration/modal_test.exs:96: (test)

These aren't failures, but are definitely noisy and do seem very liveview related:

15:35:22.362 [error] GenServer #PID<0.2433.0> terminating
** (MatchError) no match of right hand side value: :external                                                                          
    lib/phoenix_live_view/channel.ex:152: Phoenix.LiveView.Channel.call_mount_handle_params/2                                         
    lib/phoenix_live_view/channel.ex:443: Phoenix.LiveView.Channel.verified_mount/9                                                   
    (stdlib) gen_server.erl:637: :gen_server.try_dispatch/4                                                                           
    (stdlib) gen_server.erl:711: :gen_server.handle_msg/6                                                                             
    (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3                                                                            
Last message: {:mount, Phoenix.LiveView.Channel}                                                                                      
15:35:22.363 [error] an exception was raised:                                                                                         
    ** (MatchError) no match of right hand side value: :external                                                                      
        lib/phoenix_live_view/channel.ex:152: Phoenix.LiveView.Channel.call_mount_handle_params/2                                     
        lib/phoenix_live_view/channel.ex:443: Phoenix.LiveView.Channel.verified_mount/9                                               
        (stdlib) gen_server.erl:637: :gen_server.try_dispatch/4                                                                       
        (stdlib) gen_server.erl:711: :gen_server.handle_msg/6                                                                         
        (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3  

And idea what's causing this? I haven't look at the liveview code yet.

And another failure that I don't think is liveview related:

  2) test enabled element should disable after close (DrabTestApp.CoreTest)
     test/integration/core_test.exs:213
     Expected false or nil, got true
     code: refute element_enabled?(find_element(:id, "core1_button"))
     arguments:

         # 1
         %Hound.Element{uuid: "0.3055531904403481-3"}

     stacktrace:
       test/integration/core_test.exs:216: (test)

     The following output was logged:
     15:36:42.960 [error] Browser reports: Drab attribute value 'wrong' is incorrect.
     
     15:36:42.960 [error] Browser reports: Drab attribute value 'wrong.' is incorrect.
     
     15:36:42.960 [error] Browser reports: Drab attribute value 'wrong:' is incorrect.
     
     15:36:42.960 [error] Browser reports: Drab attribute value ':wrong' is incorrect.
     
     15:36:42.961 [error] Browser reports: Drab attribute value '.' is incorrect.
     
     15:36:43.322 [error] Browser reports: Drab attribute value 'wrong' is incorrect.
     
     15:36:43.322 [error] Browser reports: Drab attribute value 'wrong.' is incorrect.
     
     15:36:43.322 [error] Browser reports: Drab attribute value 'wrong:' is incorrect.
     
     15:36:43.323 [error] Browser reports: Drab attribute value ':wrong' is incorrect.
     
     15:36:43.323 [error] Browser reports: Drab attribute value '.' is incorrect.

So it looks like the liveview test is printing error messages, something not being cleaned up perhaps.

As for the two actual failures, they don't look related to this liveview work, though definitely need to look at those.

Would you be able to fix the middle message that isn't a failure but that prints out death messages about liveview stuff? :-)

@OvermindDL1
Copy link
Collaborator

Not liveview related changes post:

Some more testing shows that the first error at test/integration/modal_test.exs:92 sometimes fails and sometimes succeeds, a race condition I'm guessing.

The second error of test/integration/core_test.exs:213 is failing every time however (at least the 6 times of running it). Any ideas @grych?

@guidotripaldi
Copy link
Contributor Author

guidotripaldi commented Aug 17, 2019

some tests aren't passing, I'm looking over them currently... Test seed: 509794

I cannot find seed 509794, to which build job is it related? I can find an "test Drab.Modal modal over modal (DrabTestApp.ModalTest)" error only in job #356.2, but it seems to be caused by the peculiar elixir/erlang versions combination. Same consideration for the other Travis errors. So we could just change the travis configuration in .travis.yml adding the, currently missing, excludes matrix, something like that (not tried yet, copied from phoenix ):

elixir:
  - 1.6
  - 1.7
  - 1.8
  - 1.9
otp_release:
  - 20.3
  - 21.0
  - 22.0
matrix:
  exclude:
    - elixir: 1.6
      otp_release: 21.0
    - elixir: 1.6
      otp_release: 22.0
    - elixir: 1.7
      otp_release: 19.3
    - elixir: 1.7
      otp_release: 20.3
    - elixir: 1.8
      otp_release: 19.3
    - elixir: 1.8
      otp_release: 20.3
    - elixir: 1.9
      otp_release: 19.3
    - elixir: 1.9
      otp_release: 20.3

@OvermindDL1
Copy link
Collaborator

509794

Oh the seed number is the mix test rng seed for ordering. It doesn't matter for the liveview stuff being printed, it happens every time as far as I saw.

error only in job #356.2, but it seems to be caused by the peculiar elixir/erlang versions combination.

Interesting... I'm running 1.8.1 on OTP 21.

So we could just change the travis configuration in .travis.yml adding the, currently missing, excludes matrix, something like that (not tried yet, copied from phoenix)

It sucks even more because it seems to be a race condition, always so hard to diagnose...

@guidotripaldi
Copy link
Contributor Author

It sucks even more because it seems to be a race condition, always so hard to diagnose...

It seems an erratic problem peculiar to Travis, independent from the changes we made on Drab

@OvermindDL1
Copy link
Collaborator

I mean more related to some of the existing failures unrelated to this PR, they happen sometimes locally, but not others. ^.^;

The LV error messages, no test failure, probably just something not being cleaned up after tests?

@guidotripaldi
Copy link
Contributor Author

I don't understand why Travis complaints. Locally I never get across those errors when testing with that elixir/erlang version combination. Maybe its platform dependent, but it shouldn't. Any hints?

elixir/erlang versions combination from LiveView
@OvermindDL1
Copy link
Collaborator

I don't understand why Travis complaints. Locally I never get across those errors when testing with that elixir/erlang version combination. Maybe its platform dependent, but it shouldn't. Any hints?

It's with the tests, the LiveView socket isn't being cleaned up properly still is what these mean:

13:16:45.127 [error] GenServer #PID<0.2335.0> terminating
** (MatchError) no match of right hand side value: :external
    lib/phoenix_live_view/channel.ex:152: Phoenix.LiveView.Channel.call_mount_handle_params/2
    lib/phoenix_live_view/channel.ex:443: Phoenix.LiveView.Channel.verified_mount/9
    (stdlib) gen_server.erl:637: :gen_server.try_dispatch/4
    (stdlib) gen_server.erl:711: :gen_server.handle_msg/6
    (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3
Last message: {:mount, Phoenix.LiveView.Channel}
13:16:45.129 [error] an exception was raised:
    ** (MatchError) no match of right hand side value: :external
        lib/phoenix_live_view/channel.ex:152: Phoenix.LiveView.Channel.call_mount_handle_params/2
        lib/phoenix_live_view/channel.ex:443: Phoenix.LiveView.Channel.verified_mount/9
        (stdlib) gen_server.erl:637: :gen_server.try_dispatch/4
        (stdlib) gen_server.erl:711: :gen_server.handle_msg/6
        (stdlib) proc_lib.erl:249: :proc_lib.init_p_do_apply/3

And this error is some issue with the embedded browser testing, which I've not done much of yet, so I'm unsure. Anyone have ideas?

  1) test enabled element should disable after close (DrabTestApp.CoreTest)
     test/integration/core_test.exs:213
     Expected false or nil, got true
     code: refute element_enabled?(find_element(:id, "core1_button"))
     arguments:
         # 1
         %Hound.Element{uuid: "0.08030434519235863-3"}
     stacktrace:
       test/integration/core_test.exs:216: (test)
     The following output was logged:
     13:17:32.873 [error] Browser reports: Drab attribute value 'wrong' is incorrect.
     
     13:17:32.873 [error] Browser reports: Drab attribute value 'wrong.' is incorrect.
     
     13:17:32.873 [error] Browser reports: Drab attribute value 'wrong:' is incorrect.
     
     13:17:32.873 [error] Browser reports: Drab attribute value ':wrong' is incorrect.
     
     13:17:32.873 [error] Browser reports: Drab attribute value '.' is incorrect.
     
     13:17:33.221 [error] Browser reports: Drab attribute value 'wrong' is incorrect.
     
     13:17:33.221 [error] Browser reports: Drab attribute value 'wrong.' is incorrect.
     
     13:17:33.221 [error] Browser reports: Drab attribute value 'wrong:' is incorrect.
     
     13:17:33.221 [error] Browser reports: Drab attribute value ':wrong' is incorrect.
     
     13:17:33.221 [error] Browser reports: Drab attribute value '.' is incorrect.

@OvermindDL1
Copy link
Collaborator

Any thoughts as to the sporadic error that pops up from liveview in the tests?

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.

2 participants