daddyz / phonelib

Ruby gem for phone validation and formatting using google libphonenumber library data
MIT License
1.04k stars 130 forks source link

Only strip 00 prefix when country is not specified #268

Closed c960657 closed 1 year ago

c960657 commented 1 year ago

Years ago the company I work for had some performance issues with phonelib (I don't know the details of these problems). We forked the library and made a simple fix. This is an attempt to upstream a modified version of our changes.

The point of this change is to reduce the number of string concatenations.

Two places in code the 00 international prefix is hardcoded in addition to the country-specific international prefixes. As far as I understand, it is only relevant to hardcode this when no country is passed to Phonelib.parse. Stripping this prefix in detect_and_parse allows us to check for country codes using start_with? instead of using regexp built from concatenated strings.

Benchmark code

Phonelib.parse('4571999999')

MemoryProfiler.report do
  40.times { Phonelib.parse('4571999999') }
end.pretty_print

Results on master

$ ruby profile.rb |grep -A7 'allocated objects by clas'
allocated objects by class
-----------------------------------
     61720  String
      1800  Array
       320  MatchData
       200  Hash
        40  Phonelib::Phone

Results with this PR

$ ruby profile.rb |grep -A7 'allocated objects by clas'
allocated objects by class
-----------------------------------
      2440  String
      1800  Array
       280  MatchData
       200  Hash
        40  Phonelib::Phone

In addition to the change in stirng allocations, this patch also reduces execution time with about 70%.

ElMassimo commented 1 year ago

This PR looks great!


We're also running into performance issues when migrating from global_phone.

Currently, when parsing international phone numbers this library allocates a significant amount of regexes (256 countries), and will unnecessarily match against all 256, although in practice it can only match a maximum of 1.

Implemented a 10x speed up in:

daddyz commented 1 year ago

more changes done in #274 , I am closing this PR

ElMassimo commented 1 year ago

This pull request would still improve performance even further, as it reduces the amount of regexes used.

Please consider reopening it 😃