Fix lookup performance (again) by corny · Pull Request #150 · micke/valid_email2

corny · 2020-06-13T10:29:21Z

regression by c92c566
closes #137

dmke · 2020-06-13T11:05:07Z

+    list    = ValidEmail2.disposable_emails
+    address = ValidEmail2::Address.new("test@example.com")
+
+    expect { address.send(:domain_is_in?, list) }.to perform_under(0.0001).sample(10).times


You shouldn't assume the performance of the testing system like this. If I run the tests on a potato (or any other system with high load, such as CI systems), this will fail.

It would be better to just test whether the returned value from .whitelist / .blacklist is to be_kind_of Set. You can then assume log access time on these objects.

I rewrote the test to do a blackbox test and to check the performance of disposable_domain? directly. There are many things that can be fucked up, because the method wants to do other stuff in the future, like happened in the past.

I added a check to test the kind of the list.

check the performance of disposable_domain? directly

By setting a hard limit on 0.1ms this won't get you anywhere because you're measuring execution time. Have a look at perform_logarithmic to check complexity.

Regression by c92c566 closes micke#137

micke

I'm truly sorry for this regression and i really appreciate your contributions!
I have some suggestions, and i would also suggest you are a bit kinder when working in open-source projects :)

micke · 2020-06-13T19:23:05Z

-            # Address may not contain a dot directly before @
-            address.address !~ /\.@/


What's your reason for removing this check? It was introduced in #136

micke · 2020-06-13T19:23:26Z

-      @valid ||= begin
-        return false if @parse_error
+      return @valid if @valid != nil
+      return false  if @parse_error


Suggested change

return false if @parse_error

return false if @parse_error

micke · 2020-06-13T19:24:33Z

            # Domain needs to have at least one dot
-            domain =~ /\./ &&
+            domain.include?('.') &&
            # Domain may not have two consecutive dots
-            domain !~ /\.{2,}/ &&
+            !domain.include?('..') &&
            # Domain may not start with a dot
-            domain !~ /^\./ &&
+            !domain.start_with?('.') &&
            # Domain may not start with a dash
-            domain !~ /^-/ &&
+            !domain.start_with?('-') &&
            # Domain name may not end with a dash


I like this change, but what do you think about removing these comments? They become a bit superfluous.

micke · 2020-06-13T19:30:57Z

    def valid?
-      @valid ||= begin
-        return false if @parse_error
+      return @valid if @valid != nil


Suggested change

return @valid if @valid != nil

return @valid unless @valid.nil?

micke · 2020-06-13T20:03:03Z

+    @whitelist ||= load_if_exists(WHITELIST_FILE)
+  end
+
+private


private does not make class methods private in this context. It needs to be in a class << self or you can use private_class_method

micke · 2020-06-13T20:42:35Z

+      result = Set.new
+      File.open(path, "r").each_line do |line|
+        line.strip!
+        result << line if line.present?
+      end
+      result


Suggested change

result = Set.new

File.open(path, "r").each_line do |line|

line.strip!

result << line if line.present?

end

result

File.open(path, "r").each_line.each_with_object(Set.new) do |domain, set|

set << domain.tap(&:chomp!)

end

Calculating ------------------------------------- corny 4.858M memsize ( 0.000 retained) 67.367k objects ( 0.000 retained) 50.000 strings ( 0.000 retained) micke 4.859M memsize ( 0.000 retained) 67.367k objects ( 0.000 retained) 50.000 strings ( 0.000 retained)

user system total real corny: 23.706184 0.328159 24.034343 ( 24.114417) micke: 16.692115 0.341454 17.033569 ( 17.130394)

micke · 2020-06-13T20:43:10Z

+require "spec_helper"
+
+describe "Performance testing" do
+


Suggested change

micke · 2020-06-13T20:43:17Z

+    # check lookup timing
+    expect { address.disposable_domain? }.to perform_under(0.0001).sample(10).times
+  end
+


Suggested change

micke · 2020-06-13T20:50:59Z

+
+  let(:disposable_domain) { ValidEmail2.disposable_emails.first }
+
+  it "lookup timing" do


Suggested change

it "lookup timing" do

it "has acceptable lookup performance" do

micke · 2020-06-13T20:51:57Z

+    # This method MUST return a Set, otherwise the
+    # performance will be as bad as hell!


Suggested change

# This method MUST return a Set, otherwise the

# performance will be as bad as hell!

This is clear because of the specs.

micke · 2020-08-10T12:32:15Z

Thanks @corny and @dmke for this PR, since i didn't hear from you in a while and i want to fix this performance issue i "hijacked" your PR and fixed the last remaining issues. Hope you don't mind!

micke · 2020-08-10T12:35:12Z

I just pushed version 3.3.1 to rubygems with this regression fixed!

corny · 2020-08-10T12:40:43Z

Thanks a lot!

corny force-pushed the performance-fix branch 4 times, most recently from 31ff504 to 994ba6f Compare June 13, 2020 10:58

dmke suggested changes Jun 13, 2020

View reviewed changes

corny force-pushed the performance-fix branch 2 times, most recently from bd6545d to 6d7b91c Compare June 13, 2020 11:13

Fix lookup performance (again)

21b5116

Regression by c92c566 closes micke#137

corny force-pushed the performance-fix branch from 4954cc7 to a72e4cf Compare June 13, 2020 11:27

corny added 3 commits June 13, 2020 13:28

Add benchmark specs

f987e2a

Don't do any regexp tests for disposable_domain?

7329eb2

Rewrite valid? and fix specs

8d3b362

corny force-pushed the performance-fix branch from a72e4cf to 8d3b362 Compare June 13, 2020 11:29

micke requested changes Jun 13, 2020

View reviewed changes

micke mentioned this pull request Aug 10, 2020

Performance fix #161

Merged

micke closed this in #161 Aug 10, 2020

		# Address may not contain a dot directly before @
		address.address !~ /\.@/

	return @valid if @valid != nil
	return @valid unless @valid.nil?


		let(:disposable_domain) { ValidEmail2.disposable_emails.first }

		it "lookup timing" do

	it "lookup timing" do
	it "has acceptable lookup performance" do

		# This method MUST return a Set, otherwise the
		# performance will be as bad as hell!

Conversation

corny commented Jun 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micke commented Aug 10, 2020

Uh oh!

micke commented Aug 10, 2020

Uh oh!

corny commented Aug 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

corny commented Jun 13, 2020 •

edited

Loading