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 concurrent-ruby dependency #137

Merged
merged 1 commit into from
Jun 19, 2019

Conversation

dmagliola
Copy link
Collaborator

@dmagliola dmagliola commented Jun 17, 2019

We added this dependency to be able to use ReadWriteLock, which is
a reentract lock. We did not need the Read/Write locking distinction,
and at the time, we didn't know that Monitor was reentrant, so this
seemed to be the best solution.

Knowing that Monitor is reentrant, we can get rid of the dependency,
and simply use Monitor instead.

Note that we keep concurrent-ruby as a dev dependency, because the
performance benchmarks use other primitives provided by it to synvhronize
their threads, but it's not needed for specs to pass, or for production use.

As an added bonus, this appears to be measurably faster on multiple thread scenarios.
I only ran a quick benchmark on my laptop, so this is to be taken with a grain of salt,
but at least it doesn't seem to be worse.

@dmagliola dmagliola requested a review from Sinjo June 17, 2019 16:57
@dmagliola dmagliola mentioned this pull request Jun 17, 2019
We added this dependency to be able to use `ReadWriteLock`, which is
a reentract lock. We did not need the Read/Write locking distinction,
and at the time, we didn't know that `Monitor` was reentrant, so this
seemed to be the best solution.

Knowing that `Monitor` is reentrant, we can get rid of the dependency,
and simply use `Monitor` instead.

Note that we keep `concurrent-ruby` as a *dev* dependency, because the
performance benchmarks use other primitives provided by it to synvhronize
their threads, but it's not needed for specs to pass, or for production use.

Signed-off-by: Daniel Magliola <danielmagliola@gocardless.com>
@dmagliola dmagliola force-pushed the use_monitor_instead_of_read_write_lock branch from 83d92f6 to 6401945 Compare June 17, 2019 17:01
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6401945 on use_monitor_instead_of_read_write_lock into 8d0264f on master.

Copy link
Member

@Sinjo Sinjo left a comment

Choose a reason for hiding this comment

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

LGTM

Any chance you could drop the benchmarks in here for posterity?

@dmagliola
Copy link
Collaborator Author

Sure.

TAKE THESE WITH A MASSIVE GRAIN OF SALT, however. As we've learned, benchmarking in my local dev laptop.

master (Using ReadWriteLock):

Observe NoopStore                 x1            0.980000   0.020000   1.000000 (  1.005403)
Export  NoopStore                 x1            0.010000   0.000000   0.010000 (  0.000381)
Observe SingleThreaded            x1            4.020000   0.030000   4.050000 (  4.053466)
Export  SingleThreaded            x1            0.010000   0.000000   0.010000 (  0.005801)
Observe Synchronized              x1            7.530000   0.020000   7.550000 (  7.555869)
Export  Synchronized              x1            0.010000   0.000000   0.010000 (  0.009358)
Observe DirectFileStore           x1           23.120000   9.510000  32.630000 ( 32.722354)
Export  DirectFileStore           x1            0.020000   0.030000   0.050000 (  0.046111)
--------------------------------------------------------------------------------
Observe NoopStore                 x2            1.020000   0.010000   1.030000 (  1.031123)
Export  NoopStore                 x2            0.000000   0.000000   0.000000 (  0.000357)
Observe Synchronized              x2            7.690000   0.030000   7.720000 (  7.710281)
Export  Synchronized              x2            0.010000   0.000000   0.010000 (  0.007705)
Observe DirectFileStore           x2           32.740000  22.990000  55.730000 ( 40.354634)
Export  DirectFileStore           x2            0.020000   0.020000   0.040000 (  0.050319)
--------------------------------------------------------------------------------
Observe NoopStore                 x4            1.220000   0.020000   1.240000 (  1.239401)
Export  NoopStore                 x4            0.000000   0.000000   0.000000 (  0.000408)
Observe Synchronized              x4            7.850000   0.100000   7.950000 (  7.902657)
Export  Synchronized              x4            0.010000   0.000000   0.010000 (  0.006651)
Observe DirectFileStore           x4           33.870000  23.050000  56.920000 ( 41.797133)
Export  DirectFileStore           x4            0.030000   0.020000   0.050000 (  0.058778)
--------------------------------------------------------------------------------
Observe NoopStore                 x8            1.290000   0.000000   1.290000 (  1.301568)
Export  NoopStore                 x8            0.000000   0.000000   0.000000 (  0.000304)
Observe Synchronized              x8            7.940000   0.090000   8.030000 (  7.973229)
Export  Synchronized              x8            0.010000   0.000000   0.010000 (  0.006290)
Observe DirectFileStore           x8           34.080000  22.330000  56.410000 ( 42.073212)
Export  DirectFileStore           x8            0.020000   0.030000   0.050000 (  0.047755)

This Branch (using Monitor):

Observe NoopStore                 x1            0.960000   0.020000   0.980000 (  0.984010)
Export  NoopStore                 x1            0.000000   0.000000   0.000000 (  0.000381)
Observe SingleThreaded            x1            4.070000   0.020000   4.090000 (  4.111630)
Export  SingleThreaded            x1            0.010000   0.000000   0.010000 (  0.007372)
Observe Synchronized              x1            4.920000   0.030000   4.950000 (  4.972645)
Export  Synchronized              x1            0.010000   0.000000   0.010000 (  0.005994)
Observe DirectFileStore           x1           18.370000   8.980000  27.350000 ( 27.470807)
Export  DirectFileStore           x1            0.030000   0.020000   0.050000 (  0.047467)
--------------------------------------------------------------------------------
Observe NoopStore                 x2            0.980000   0.010000   0.990000 (  0.996643)
Export  NoopStore                 x2            0.000000   0.000000   0.000000 (  0.000384)
Observe Synchronized              x2            4.920000   0.100000   5.020000 (  4.957631)
Export  Synchronized              x2            0.010000   0.000000   0.010000 (  0.006755)
Observe DirectFileStore           x2           26.530000  21.360000  47.890000 ( 33.575565)
Export  DirectFileStore           x2            0.030000   0.030000   0.060000 (  0.058696)
--------------------------------------------------------------------------------
Observe NoopStore                 x4            1.210000   0.010000   1.220000 (  1.224279)
Export  NoopStore                 x4            0.000000   0.000000   0.000000 (  0.000428)
Observe Synchronized              x4            4.970000   0.130000   5.100000 (  5.017931)
Export  Synchronized              x4            0.010000   0.000000   0.010000 (  0.005345)
Observe DirectFileStore           x4           26.360000  18.890000  45.250000 ( 33.632237)
Export  DirectFileStore           x4            0.030000   0.020000   0.050000 (  0.063235)
--------------------------------------------------------------------------------
Observe NoopStore                 x8            1.380000   0.020000   1.400000 (  1.398984)
Export  NoopStore                 x8            0.000000   0.000000   0.000000 (  0.000262)
Observe Synchronized              x8            5.210000   0.130000   5.340000 (  5.258770)
Export  Synchronized              x8            0.010000   0.000000   0.010000 (  0.005993)
Observe DirectFileStore           x8           26.970000  21.040000  48.010000 ( 34.309391)
Export  DirectFileStore           x8            0.030000   0.030000   0.060000 (  0.049344)

@dmagliola dmagliola merged commit a902f4b into master Jun 19, 2019
@dmagliola dmagliola deleted the use_monitor_instead_of_read_write_lock branch June 19, 2019 16:41
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.

4 participants