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

don't recheck events in select_from_last_begin #296

Closed
wants to merge 12 commits into from

Conversation

ces42
Copy link
Contributor

@ces42 ces42 commented May 29, 2023

This addresses part of #295 . It fixes the inefficiency in

def select_from_last_begin
return self if empty?
index_from_last = @events.reverse.find_index { |e| e.record.status == "begin" }
return GestureBuffer.new([]) if index_from_last.nil?
index_last_begin = events.length - index_from_last - 1
GestureBuffer.new(@events[index_last_begin..-1])
end

.reverse.find has bad runtime because it creates a reversed copy of the list (in linear time) and then iterates over it. But of course one should just iterate backwards over the list.

Furthermore instead of searching the whole buffer at every call of select_from_last_begin we should just search all entries that have been added since the last call and if none of them are a "begin" we just return the same as last time. This is what the two new variables @mem_last_begin and @mem_checked are for.
When I run fusuma, the "begin" event is always at index 0 because the buffer is cleared before the next gesture starts and there's always one more entry in @events than in the last call. So this optimization is very efficient.

Finally, if the last "begin" did happen at index 0 we can return self instead of returning an identical copy.

possible problems that this could cause:
Things will break if

  • @events is modified in-place in any way other than appending events (e.g. pop())
  • select_from_last_begin returns self and that return value is modified

profiling
I enabled the line profiler and ran master vs my fork while (3-finger) swiping from one end of my touchpad to the other end (separate swipes left, right, left, right, etc.) with time exe/fusuma

current master:

lib/fusuma/plugin/detectors/swipe_detector.rb                                                                                                                                 
               |  18          # @return [NilClass] if event is NOT detected
               |  19          def detect(buffers)
   3.4ms  2682 |  20            gesture_buffer = buffers.find { |b| b.type == BUFFER_TYPE }
 315.3ms   894 |  21              .select_from_last_begin
 462.7ms 302172 |  22              .select_by_events { |e| e.record.gesture == GESTURE_RECORD_TYPE }
               |  23  
 310.4ms   894 |  24            updating_events = gesture_buffer.updating_events
   0.4ms   894 |  25            return if updating_events.empty?

this fork:

lib/fusuma/plugin/detectors/swipe_detector.rb
               |  18          # @return [NilClass] if event is NOT detected
               |  19          def detect(buffers)
   3.2ms  2745 |  20            gesture_buffer = buffers.find { |b| b.type == BUFFER_TYPE }
   9.3ms   915 |  21              .select_from_last_begin
 654.5ms 449571 |  22              .select_by_events { |e| e.record.gesture == GESTURE_RECORD_TYPE }
               |  23  
 454.9ms   915 |  24            updating_events = gesture_buffer.updating_events
   0.5ms   915 |  25            return if updating_events.empty?

@ces42 ces42 force-pushed the optimize branch 5 times, most recently from d3458bd to e479134 Compare May 29, 2023 22:54
@ces42 ces42 force-pushed the optimize branch 4 times, most recently from 4e48875 to d8b7064 Compare May 30, 2023 07:47
Also cache `GestureBuffer.updating_events` and selecting events by
gesture type (introduce `GestureBuffer.select_by_type` for this)
Comment on lines +97 to +110
# conditions(&block).find do |c, l|
# result = l.call
# return [c, result] if result

nil
end
# nil
# end
# That code is equivalent to
result = block.call
return [:nothing, result] if result
# result = Config::Searcher.skip(&block).call
@skip = true
result = block.call
@skip = false
return [:skip, result] if result
Copy link
Owner

Choose a reason for hiding this comment

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

Did you leave a comment for the purpose of understanding the code?
Let's either revert the commented-out changes or remove comments.

cache_entry.value + upd_ev[-1].record.delta[attr].to_f - \
(upd_ev.length > 10 ? upd_ev[-11].record.delta[attr].to_f : 0)
else
return cache_entry.value
Copy link
Owner

Choose a reason for hiding this comment

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

https://github.com/iberianpig/fusuma/actions/runs/5491536218/jobs/10008145456?pr=296

In the CI, there is an error occurring with Ruby 2.5.
It appears to be due to a mix of assignment and return statements in the branching of an if-else clause.

# @return [Array]
def all
@all ||= if @name_patterns.empty?
@accept_any = true
Copy link
Owner

Choose a reason for hiding this comment

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

It appears that the use of @accept_any = true is causing the filtering by Device.available to be ineffective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. Is this an issue? I assumed that the use of filtering would be "negative filtering", i.e. something like "don't make fusuma try to process my touchscreen events as gestures". It seems that if there's no name_patterns then there is no need to filter events? I also expected that events would always come from "available" devices, is that wrong?

Copy link
Owner

Choose a reason for hiding this comment

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

This is actually filtering to extract only touchpad devices. If there is no filtering, all devices including keyboards will be included.

We need to filter the events in order to limit them to touchpad devices only. By default, when name_patterns is empty, Device.available is used to select automatically touchpad devices. It finds devices with gesture capabilities and the ability to perform natural scrolling, which are typically touchpad devices.

If it is set name_patterns (specified explicitly by the user), fusuma keep only specified touchpad device.

@iberianpig
Copy link
Owner

@ces42
I apologize for the delayed review of the pull request.
Thank you for tuning the performance.
I have made some comments.

@ces42
Copy link
Contributor Author

ces42 commented Jul 10, 2023

Hi! I'm traveling right now so I probably won't have time to look at this until next week. I have made some more changes (locally) and I split the ones here into three separate branches, maybe that will make reviewing easier.

@ces42 ces42 mentioned this pull request Jul 17, 2023
@ces42 ces42 closed this Jul 17, 2023
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