search

sillsdev / chorus

End-user collaboration library via 3-way xml merging and hg dvcs under the hood
6 stars 26 forks source link

Speed up Send/Receive operations significantly #257

Closed rmunn closed 1 year ago

rmunn commented 3 years ago

The Send/Receive code does a lot of hg log -rREV --template "{node}" calls in order to learn the long hash of a revision number. This happens very often during a Send/Receive process: the Chorus logs are full of hg log -r0 operations. Thing is, every single call to hg log incurs a cost as a Python process has to be spun up every time this happens. And in any given repository, a specific revision number wil always refer to the same revision with the same long hash, so each long hash only has to be calculated once for any given revision number. So we'll cache the output of log -rREV operations and return the cached output if the same operation runs a second time. (Other operations like hg parents aren't safe to cache, as their result will change during a Send/Receive operation as new commits are added to the repo. Only log -rREV calls are 100% safe to cache).

This change is Reviewable

rmunn commented 3 years ago

I anticipate that this will produce measurable speed improvements in Send/Receive operations on Language Forge, but I don't have benchmark numbers yet. Once I've been able to run some benchmarks I'll leave a comment reporting on what I find.

rmunn commented 3 years ago

Closing and re-opening to retrigger the LGTM analysis, as it seems like it crashed in a non-standard way.

rmunn commented 3 years ago

@ermshiperete wrote:

Should also be applied to master branch.

Done: #258.

I can't merge this as the LGTM analysis is failing to build and so the "Squash and merge" button is disabled for me (and I'm not a repo admin so I can't override the disabled button). I've tried re-running the LGTM analysis check but it keeps failing to build. Can you merge it?

rmunn commented 3 years ago

Never mind, I just figured out that the reason the build is failing is actually a compiler error: I failed to notice that the ExecuteErrorsOk method where I put the call is a static method, so I either have to pass the cache in as a parameter or move the caching check to an instance method. (I'm having trouble building Chorus locally so I didn't see the compiler error until I saw the TeamCity build). I'll submit a new version that actually compiles.

lgtm-com[bot] commented 3 years ago

This pull request introduces 1 alert when merging 79960996b5ed3831ede81d2701a5c7fd93e5d357 into 6646a161acf83a2541a25404313b781bc5a3fe88 - view on LGTM.com

new alerts:

rmunn commented 3 years ago

I've merged https://github.com/sillsdev/chorus/pull/258, but the Jenkins status checks aren't reporting to GitHub for some reason so I can't do a squash-and-merge here. @ermshiperete, do you have admin access to the repo and therefore the ability to override this merge? If so, could you please copy the commit message I used in #258? Thanks.

rmunn commented 3 years ago

The Jenkins build isn't running, and I've run out of time for today to figure out which ref to build. I've tried refs/pull/257/merge and pr/257/merge, and neither one worked. I'll have to leave that to someone else to figure out.

megahirt commented 1 year ago

Closing as a duplicate of #258 . The lfmerge branch is no longer in use.