Closed verdverm closed 3 years ago
myitcv
commented
3 years ago @verdverm - thanks for bisecting.
Per https://github.com/cuelang/cue/wiki/Creating-test-or-performance-reproducers, please can you provide a more detailed reproducer, with a specific commit of the hof project (so that we don't accidentally pick up a workaround/any other artefacts)? Thanks
verdverm
commented
3 years ago added to the original writeup
myitcv
commented
3 years ago That repro doesn't work for me:
cd $(mktemp -d)
(
set -ex
# get repro
git clone https://github.com/hofstadter-io/_saas
cd _saas
# get hof
curl https://github.com/hofstadter-io/hof/releases/download/v0.5.15/hof_0.5.15_Linux_x86_64 -o hof
chmod +x ./hof
# fetch deps
./hof mod vendor cue
# run repro
cue eval
)Gives:
./hof: line 1: syntax error near unexpected token `<'
./hof: line 1: `<html><body>You are being <a href="https://github-production-release-asset-2e65be.s3.amazonaws.com/238593012/ff3dfa00-5346-11eb-80e2-53bed5772806?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20210125%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20210125T155716Z&X-Amz-Expires=300&X-Amz-Signature=d30cc815ac8a0f1efed4090076909e05238675c0d4f439158af44d57deb1cd4c&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=238593012&response-content-disposition=attachment%3B%20filename%3Dhof_0.5.15_Linux_x86_64&response-content-type=application%2Foctet-stream">redirected</a>.</body></html>'But the following does (added missing -L flag to curl):
cd $(mktemp -d)
(
set -ex
# get repro
git clone https://github.com/hofstadter-io/_saas
cd _saas
# get hof
curl -sL https://github.com/hofstadter-io/hof/releases/download/v0.5.15/hof_0.5.15_Linux_x86_64 -o hof
chmod +x ./hof
# fetch deps
./hof mod vendor cue
# run repro
cue eval
)Per my original question, what commit of the _sass repo should we be checking? Your repro is the tip of the default remote branch... which is a moving feast.
verdverm
commented
3 years ago I pushed earlier and made sure everything is current for this repro. Won't be touching any of it for a bit, so tip there should be good.
myitcv
commented
3 years ago Won't be touching any of it for a bit, so tip there should be good.
That might well be, but communicating the exact commit with which you tested is as much about confirming from your side that you were indeed testing a clean commit, as opposed to a dirty working tree. It also means that you can copy-paste the repro steps you are sharing locally to verify as a quick final check that they a) work and b) are using exactly the right code.
Perhaps most importantly it also makes the repro consistent over time, and hence reduces the amount of effort/potential confusion for someone looking to investigate now or later.
mpvl
commented
3 years ago @verdverm could you verify whether this is fixed with master?
verdverm
commented
3 years ago @mpvl Can confirm the panic is gone, but with a decent slowdown (~2.5s -> ~4.5) (previous is beta.1)
mpvl
commented
3 years ago @verdverm Would you be able to bisect at which commit the slowdown occurs?
mpvl
commented
3 years ago Or alternatively, see if the slowdown still occurs when commenting out the body of addNotify
if v != nil {
n.notify = append(n.notify, v)
}at around internal/core/adt/eval.go:732?
verdverm
commented
3 years ago Commenting out that code does not later the runtime. I'll get to bisecting next
verdverm
commented
3 years ago Not sure I can bisect due to the panics.
So I do not know if / where the slowdown occurred between beta.1 and ee78ec3
myitcv
commented
3 years ago On the basis the panic reported in this issue is now solved, I think we need to move performance issues to a separate issue.
It's not clear to me that we have a solid base against which to compare performance here in any case. Reason being, there a correctness fixes along the way. The panic was "introduced" in bef7b263893bf64368d00aeb6d3e8cf7976dbdf2 which is fixing a cycle bug for example.
That's not to say performance issues aren't important. Far from it. Rather it's hard to definitively say there has been a real performance regression here, and instead that we should, all else being equal, come to look in more detail at a later time.
cueckoo
commented
3 years ago This issue has been migrated to https://github.com/cue-lang/cue/issues/673.
For more details about CUE's migration to a new home, please see https://github.com/cue-lang/cue/issues/1078.
What version of CUE are you using (
cue version)?occurs with: beta.2 & beta.3
ok in beta.1
bisected to a086f74
Stack trace
cue eval(with https://github.com/hofstadter-io/_saas) (seeing the same stack via the Go API in hof)Repro
Linux64 repro steps