Memoize to reduce web requests to external sites like GitHub. #1101

Thorium · 2015-09-30T16:34:30Z

I had some problems with GitHub:
System.Net.WebException: The remote server returned an error: (403) Forbidden.
"API rate limit exceeded"
So after this it shouldn't make duplicate network calls when resolving versions inside one Paket-command (like paket outdated or paket update).

…b. Note that Paket uses parallel downloads with async.

forki · 2015-09-30T16:38:02Z

did you see where it repeatably calls the same url?

Thorium · 2015-09-30T18:13:20Z

No, I didn't try to track with Fiddler.

If I blindly add counter to this memoize function, it will be hit 239 times with this paket.dependencies:

    source https://www.nuget.org/api/v2

    nuget FAKE
    nuget FSharp.Formatting
    nuget FSharp.Data
    nuget Microsoft.AspNet.SignalR.Core
    nuget Microsoft.AspNet.WebApi.OwinSelfHost
    nuget Microsoft.AspNet.Identity.Owin
    nuget Microsoft.Owin.Cors
    nuget Microsoft.Owin.StaticFiles
    nuget Microsoft.Owin.Diagnostics
    nuget Microsoft.Owin.Security.Facebook
    nuget Microsoft.Owin.Security.Google
    nuget Owin.Security.AesDataProtectorProvider
    nuget MySql.Data
    nuget Rx-Main
    nuget SQLProvider
    nuget SourceLink.Fake

    github zurb/bower-foundation css/foundation.css
    github zurb/bower-foundation css/normalize.css
    github zurb/bower-foundation js/foundation.min.js
    github SignalR/bower-signalr jquery.signalR.js
    github aFarkas/html5shiv dist/html5shiv.min.js
    github reactjs/react-bower react.js
    github FortAwesome/Font-Awesome css/font-awesome.min.css
    github FortAwesome/Font-Awesome fonts/fontawesome-webfont.eot
    github FortAwesome/Font-Awesome fonts/fontawesome-webfont.svg
    github FortAwesome/Font-Awesome fonts/fontawesome-webfont.ttf
    github FortAwesome/Font-Awesome fonts/fontawesome-webfont.woff
    github FortAwesome/Font-Awesome fonts/fontawesome-webfont.woff2
    github danielm/uploader src/dmuploader.min.js
    github danielm/uploader demos/js/demo-preview.js
    github booncon/slippry dist/slippry.css
    github booncon/slippry dist/slippry.min.js
    github booncon/slippry images/arrows.svg
    github booncon/slippry images/sy-loader.gif
    github lodash/lodash lodash.min.js

    http http://ajax.aspnetcdn.com/ajax/jQuery/jquery-2.1.4.min.js jquery.min.js
    http https://code.jquery.com/ui/1.10.4/jquery-ui.min.js
    http https://code.jquery.com/ui/1.10.4/themes/sunny/jquery-ui.min.css

239 requests are not so much but if those (or like 20% of them) are happening at exact the same time to the same web-site, the firewall may not like that. I assume that this .NET concurrent dictionary will not just remove the dublicate calls, but also reduce the parallel request count by slowing down the process a few milliseconds (as it syncs between the threads) which may also help GitHub to not block the user.

forki · 2015-09-30T18:31:06Z

For me that sounds like you found another perf issue. Will investigate
tomorrow. Can't believe it's correct to make so many calls.
On Sep 30, 2015 8:13 PM, "Tuomas Hietanen" notifications@github.com wrote:

No, I didn't try to track with Fiddler.

If I blindly add counter to this memoize function, it will be hit 239
times with this paket.dependencies:

source https://www.nuget.org/api/v2

nuget FAKE
nuget FSharp.Formatting
nuget FSharp.Data
nuget Microsoft.AspNet.SignalR.Core
nuget Microsoft.AspNet.WebApi.OwinSelfHost
nuget Microsoft.AspNet.Identity.Owin
nuget Microsoft.Owin.Cors
nuget Microsoft.Owin.StaticFiles
nuget Microsoft.Owin.Diagnostics
nuget Microsoft.Owin.Security.Facebook
nuget Microsoft.Owin.Security.Google
nuget Owin.Security.AesDataProtectorProvider
nuget MySql.Data
nuget Rx-Main
nuget SQLProvider
nuget SourceLink.Fake

github zurb/bower-foundation css/foundation.css
github zurb/bower-foundation css/normalize.css
github zurb/bower-foundation js/foundation.min.js
github SignalR/bower-signalr jquery.signalR.js
github aFarkas/html5shiv dist/html5shiv.min.js
github reactjs/react-bower react.js
github FortAwesome/Font-Awesome css/font-awesome.min.css
github FortAwesome/Font-Awesome fonts/fontawesome-webfont.eot
github FortAwesome/Font-Awesome fonts/fontawesome-webfont.svg
github FortAwesome/Font-Awesome fonts/fontawesome-webfont.ttf
github FortAwesome/Font-Awesome fonts/fontawesome-webfont.woff
github FortAwesome/Font-Awesome fonts/fontawesome-webfont.woff2
github danielm/uploader src/dmuploader.min.js
github danielm/uploader demos/js/demo-preview.js
github booncon/slippry dist/slippry.css
github booncon/slippry dist/slippry.min.js
github booncon/slippry images/arrows.svg
github booncon/slippry images/sy-loader.gif
github lodash/lodash lodash.min.js

http http://ajax.aspnetcdn.com/ajax/jQuery/jquery-2.1.4.min.js jquery.min.js
http https://code.jquery.com/ui/1.10.4/jquery-ui.min.js
http https://code.jquery.com/ui/1.10.4/themes/sunny/jquery-ui.min.css

239 requests are not so much but if those (or like 20% of them) are
happening at exact the same time to the same web-site, the firewall may not
like that. I assume that this .NET concurrent dictionary will not just
remove the dublicate calls, but also reduce the parallel request count by
slowing down the process a few milliseconds (as it syncs between the
threads) so that may also help GitHub to not block the user.

—
Reply to this email directly or view it on GitHub
#1101 (comment).

forki · 2015-10-01T06:05:11Z

src/Paket.Core/Utils.fs

+let cache = ConcurrentDictionary<(string * obj) option,Lazy<obj>>()
+let memoizeConcurrent (caller:string) (f: ('a -> 'b)) =
+    fun (x :'a) -> 
+        (cache.GetOrAdd(Some (caller, x|>box), lazy ((f x)|>box)).Force() |> unbox) : 'b


why the lazy ((f x) |> box)).Force()?
why not (f x) |> box)?

Why is the key (string * obj) option and not just (string * obj)?

are you sure it's really caching?

Thorium · 2015-10-01T06:35:34Z

Lazy is due to the reason that ConcurrentDictionary is locking the threads during the update-action. So lazy will cause threads to be not locked while f is executed.

Just tuple for the key should do well also. I think that the limitation is just that key has to be IEquatable. I just took the simplest and best memoize from fssnip.net to be the base implementation and improved that.

You can test this code with Tomas Petricek's Fibonacci, just replace his non-threadsafe memoize command with memoizeConcurrent: http://www.fssnip.net/8P

forki · 2015-10-01T07:13:17Z

I was able to understand the root cause.
your deps file specifies multiple files of the same repo. so we look up the hash for every file. that's always the same hash for files of the same repo -> we can cache that.

I changed your strategy to only cache the hashes (and possible remote dependencies files). Please review ad109f3

Thorium · 2015-10-01T07:32:36Z

A bit more verbose code.
But as long as I'm not banned by GitHub, I'm happy.
So this is also fine.

forki · 2015-10-01T07:34:22Z

yeah. and it doesn't do full memoization. I don't really like that because it's harder to explain later. It's "just" cache lookup.

Anyways it saves you 19 github calls per update.

…es in order to reduce stress on API limit - references #1101

forki · 2015-10-01T09:28:16Z

I was able to reduce the calls by checking if we already downloaded the corresponding dependencies file in the right version. please review f402345

forki · 2015-10-01T10:38:31Z

(nice side effect it's also much faster for the second update)

Thorium added 2 commits September 30, 2015 19:21

Memoization cache to reduce web requests to external sites like GitHu…

6d50e1a

…b. Note that Paket uses parallel downloads with async.

Merge branch 'master' of https://github.com/fsprojects/Paket

29fa857

forki reviewed Oct 1, 2015
View reviewed changes

forki merged commit 29fa857 into fsprojects:master Oct 1, 2015

forki added a commit that referenced this pull request Oct 1, 2015

Usee concurrent dictionary without option type - references #1101

5080db2

forki added a commit that referenced this pull request Oct 1, 2015

Use concurrent dictionary directly in RemoteDownload - references #1101

ad109f3

forki added a commit that referenced this pull request Oct 1, 2015

Check if we already downloaded paket.dependencies file for remote fil…

f402345

…es in order to reduce stress on API limit - references #1101

forki added a commit that referenced this pull request Oct 1, 2015

Do not parse remote dependencies file twice - references #1101

b2dfa7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memoize to reduce web requests to external sites like GitHub. #1101

Memoize to reduce web requests to external sites like GitHub. #1101

Thorium commented Sep 30, 2015

forki commented Sep 30, 2015

Thorium commented Sep 30, 2015

forki commented Sep 30, 2015

forki Oct 1, 2015

forki Oct 1, 2015

Thorium commented Oct 1, 2015

forki commented Oct 1, 2015

Thorium commented Oct 1, 2015

forki commented Oct 1, 2015

forki commented Oct 1, 2015

forki commented Oct 1, 2015

Memoize to reduce web requests to external sites like GitHub. #1101

Memoize to reduce web requests to external sites like GitHub. #1101

Conversation

Thorium commented Sep 30, 2015

forki commented Sep 30, 2015

Thorium commented Sep 30, 2015

forki commented Sep 30, 2015

forki Oct 1, 2015

Choose a reason for hiding this comment

forki Oct 1, 2015

Choose a reason for hiding this comment

Thorium commented Oct 1, 2015

forki commented Oct 1, 2015

Thorium commented Oct 1, 2015

forki commented Oct 1, 2015

forki commented Oct 1, 2015

forki commented Oct 1, 2015