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.

Sign up for GitHub

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

Jump to bottom

remove thread-safe wrapper #1315

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

remove thread-safe wrapper #1315

wants to merge 1 commit into from

Conversation

sago35
Copy link
Member

@sago35 sago35 commented Aug 25, 2020

This PR is for confirmation purposes only.

Although the following is written in the source code, it may be possible to run it parrallel in practice.

// Due to some problems with LLD, we cannot run links in parallel, or in parallel with compiles.
// Therefore, we put a lock around builds and run everything else in parallel.
https://github.com/tinygo-org/tinygo/blob/release/main_test.go#L118

In my hand, the test has finished without any failure.

before : 47 sec
after : 8 sec

@sago35
Copy link
Member Author

sago35 commented Aug 25, 2020

log:

$ CGO_CPPFLAGS="-I/home/sago35/tinygo/tinygo/llvm-project/llvm/include -I/home/sago35/tinygo/tinygo/llvm-build/include -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/hom
e/sago35/tinygo/tinygo/llvm-build/tools/clang/include -I/home/sago35/tinygo/tinygo/llvm-project/clang/include -I/home/sago35/tinygo/tinygo/llvm-project/lld/include" CGO_CXXFLAGS="-std=c++14" CGO_LDFLAGS="/home
/sago35/tinygo/tinygo/llvm-build/lib/libclang.a -std=c++14 -L/home/sago35/tinygo/tinygo/llvm-build/lib -Wl,--start-group -lclangAnalysis -lclangARCMigrate -lclangAST -lclangASTMatchers -lclangBasic -lclangCode
Gen -lclangCrossTU -lclangDriver -lclangDynamicASTMatchers -lclangEdit -lclangFormat -lclangFrontend -lclangFrontendTool -lclangHandleCXX -lclangHandleLLVM -lclangIndex -lclangLex -lclangParse -lclangRewrite -
lclangRewriteFrontend -lclangSema -lclangSerialization -lclangStaticAnalyzerCheckers -lclangStaticAnalyzerCore -lclangStaticAnalyzerFrontend -lclangTooling -lclangToolingASTDiff -lclangToolingCore -lclangTooli
ngInclusions -Wl,--end-group -lstdc++ -Wl,--start-group -llldCOFF -llldCommon -llldCore -llldDriver -llldELF -llldMachO -llldMinGW -llldReaderWriter -llldWasm -llldYAML -Wl,--end-group -L/home/sago35/tinygo/ti
nygo/llvm-build/lib  -lLLVMOption -lLLVMMCJIT -lLLVMLTO -lLLVMPasses -lLLVMObjCARCOpts -lLLVMExtensions -lLLVMInterpreter -lLLVMFrontendOpenMP -lLLVMExecutionEngine -lLLVMRuntimeDyld -lLLVMCoverage -lLLVMCorou
tines -lLLVMipo -lLLVMInstrumentation -lLLVMVectorize -lLLVMLinker -lLLVMIRReader -lLLVMAsmParser -lLLVMAVRDisassembler -lLLVMAVRCodeGen -lLLVMAVRAsmParser -lLLVMAVRDesc -lLLVMAVRInfo -lLLVMWebAssemblyDisassem
bler -lLLVMWebAssemblyCodeGen -lLLVMWebAssemblyDesc -lLLVMWebAssemblyAsmParser -lLLVMWebAssemblyInfo -lLLVMRISCVDisassembler -lLLVMRISCVCodeGen -lLLVMRISCVAsmParser -lLLVMRISCVDesc -lLLVMRISCVUtils -lLLVMRISCV
Info -lLLVMAArch64Disassembler -lLLVMAArch64CodeGen -lLLVMAArch64AsmParser -lLLVMAArch64Desc -lLLVMAArch64Utils -lLLVMAArch64Info -lLLVMARMDisassembler -lLLVMARMCodeGen -lLLVMARMAsmParser -lLLVMARMDesc -lLLVMA
RMUtils -lLLVMARMInfo -lLLVMX86Disassembler -lLLVMX86AsmParser -lLLVMX86CodeGen -lLLVMCFGuard -lLLVMGlobalISel -lLLVMSelectionDAG -lLLVMAsmPrinter -lLLVMDebugInfoDWARF -lLLVMCodeGen -lLLVMTarget -lLLVMScalarOp
ts -lLLVMInstCombine -lLLVMAggressiveInstCombine -lLLVMTransformUtils -lLLVMBitWriter -lLLVMAnalysis -lLLVMProfileData -lLLVMX86Desc -lLLVMObject -lLLVMTextAPI -lLLVMMCParser -lLLVMBitReader -lLLVMCore -lLLVMR
emarks -lLLVMBitstreamReader -lLLVMMCDisassembler -lLLVMMC -lLLVMDebugInfoCodeView -lLLVMDebugInfoMSF -lLLVMBinaryFormat -lLLVMX86Utils -lLLVMX86Info -lLLVMSupport -lLLVMDemangle -lrt -ldl -lpthread -lm -lstdc
++ " go test -v -buildmode exe -tags byollvm . -count=1
=== RUN   TestCompiler
=== RUN   TestCompiler/Host
=== PAUSE TestCompiler/Host
=== RUN   TestCompiler/EmulatedCortexM3
=== PAUSE TestCompiler/EmulatedCortexM3
=== RUN   TestCompiler/ARMLinux
=== PAUSE TestCompiler/ARMLinux
=== RUN   TestCompiler/ARM64Linux
=== PAUSE TestCompiler/ARM64Linux
...
        --- PASS: TestCompiler/EmulatedCortexM3/math.go (1.34s)
        --- PASS: TestCompiler/EmulatedCortexM3/float.go (0.84s)
        --- PASS: TestCompiler/EmulatedCortexM3/string.go (0.84s)
        --- PASS: TestCompiler/EmulatedCortexM3/structs.go (0.83s)
        --- PASS: TestCompiler/EmulatedCortexM3/calls.go (0.87s)
        --- PASS: TestCompiler/EmulatedCortexM3/channel.go (1.01s)
        --- PASS: TestCompiler/EmulatedCortexM3/slice.go (0.88s)
        --- PASS: TestCompiler/EmulatedCortexM3/binop.go (0.78s)
        --- PASS: TestCompiler/EmulatedCortexM3/cgo (1.02s)
        --- PASS: TestCompiler/EmulatedCortexM3/atomic.go (0.73s)
        --- PASS: TestCompiler/EmulatedCortexM3/stdlib.go (3.29s)
PASS
ok      github.com/tinygo-org/tinygo    8.340s

@sago35
Copy link
Member Author

sago35 commented Aug 25, 2020 •
edited
Loading

#1299 log:

windows (azure pipelines) : 96.496s
https://dev.azure.com/tinygo/tinygo/_build/results?buildId=1995&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=7c342334-42ee-5705-4a52-64af66099390

linux (circleci) : 70.762s
https://app.circleci.com/pipelines/github/tinygo-org/tinygo/3462/workflows/9c92d137-ac88-4d33-9e41-f9603a913c41/jobs/17020

macos (circleci) : 84.500s
https://app.circleci.com/jobs/github/tinygo-org/tinygo/17022

@niaow
Copy link
Member

niaow commented Aug 25, 2020

So this should work now I think since we now self-exec to run the linker in a separate process.

@sago35
Copy link
Member Author

sago35 commented Aug 25, 2020 •
edited
Loading

Some items failed on windows only.

main.go:763: error: rename C:\Users\VssAdministrator\AppData\Local\tinygo\goroot-go1.15-789e54ede5abeee126874a6233b5eff9379b0505b353e361b7ef424e4107038d-syscall.tmp8674665223082153551 C:\Users\VssAdministrator\AppData\Local\tinygo\goroot-go1.15-789e54ede5abeee126874a6233b5eff9379b0505b353e361b7ef424e4107038d-syscall: Access is denied.
=== CONT TestCompiler/EmulatedRISCV/zeroalloc.go

https://dev.azure.com/tinygo/tinygo/_build/results?buildId=2004&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=7c342334-42ee-5705-4a52-64af66099390&l=271

@aykevl
Copy link
Member

aykevl commented Aug 26, 2020 •
edited
Loading

This Windows failure may be a race condition in loader/goroot.go, which I've fixed here: #1316

Overall I think it should be fine to run these builds in parallel. The only possible issue is CGo, but at least on Windows, CGo is already serialized because of problems in the LLVM build.

@sago35
Copy link
Member Author

sago35 commented Aug 27, 2020 •
edited
Loading

windows : test failed
https://dev.azure.com/tinygo/tinygo/_build/results?buildId=2004&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=7c342334-42ee-5705-4a52-64af66099390&l=271

linux : 35.319s <-- 70.762s
https://app.circleci.com/pipelines/github/tinygo-org/tinygo/3471/workflows/4a1ed5d8-722c-4728-9337-5aee5e3c7456/jobs/17095

macos : 58.460s <-- 84.500s
https://app.circleci.com/pipelines/github/tinygo-org/tinygo/3471/workflows/4a1ed5d8-722c-4728-9337-5aee5e3c7456/jobs/17093

@deadprogram
Copy link
Member

Do we need to remove only for windows to get this to pass Azure tests?

@sago35
Copy link
Member Author

sago35 commented Aug 27, 2020

I think the test passes by excluding windows.

Alternatively, when #1316 is resolved, I think the windows test will pass.

@aykevl
Copy link
Member

aykevl commented Aug 27, 2020

I would personally much rather fix the issue than work around it. The Windows issue looks like an actual bug, that's hopefully fixed with #1316. If that PR doesn't fix it, I'll look further into it.

@sago35
Copy link
Member Author

sago35 commented Aug 27, 2020 •
edited
Loading

It's a little less, but it still seems to be an error.

main.go:763: error: rename C:\Users\VssAdministrator\AppData\Local\tinygo\goroot-go1.15-789e54ede5abeee126874a6233b5eff9379b0505b353e361b7ef424e4107038d.tmp925366029 C:\Users\VssAdministrator\AppData\Local\tinygo\goroot-go1.15-789e54ede5abeee126874a6233b5eff9379b0505b353e361b7ef424e4107038d-syscall: Access is denied.

https://dev.azure.com/tinygo/tinygo/_build/results?buildId=2015&view=logs&j=ca395085-040a-526b-2ce8-bdc85f692774&t=7c342334-42ee-5705-4a52-64af66099390&l=277

@aykevl aykevl mentioned this pull request Aug 28, 2020
Merged
@aykevl
Copy link
Member

aykevl commented Aug 28, 2020

I tested on Windows and found the underlying issue(s): #1325

@deadprogram
Copy link
Member

@sago35 you should be able to rebase the dev branch into this branch, and it should now pass all CI tests. Thanks!

@aykevl
Copy link
Member

aykevl commented Aug 29, 2020

I'm not sure why the test failed, but it might be a concurrency issue.

@sago35 sago35 mentioned this pull request Mar 18, 2021
Merged
@aykevl
Copy link
Member

aykevl commented Apr 6, 2021

@sago35 can you maybe try rebasing this on the dev branch? I think it will work now.

@sago35
Copy link
Member Author

sago35 commented Apr 6, 2021

@sago35
Copy link
Member Author

sago35 commented Apr 6, 2021

@aykevl
It seems that the test still fails sometimes.
The error log I could check is below.

  • build-linux : 5m40s (failed)
  • build-macos : 2m11s
  • windows : 4m1s

build-linux log:

    main.go:802: error: rename /home/circleci/.cache/tinygo/picolibc-armv7m-none-eabi.a.tmp /home/circleci/.cache/tinygo/picolibc-armv7m-none-eabi.a: no such file or directory

@deadprogram
Copy link
Member

"Infrastructure fail"??!

image

@aykevl
Copy link
Member

aykevl commented Apr 6, 2021

Huh, that are some weird errors. Maybe it's best to simply retry the build.

@niaow
Copy link
Member

niaow commented Dec 28, 2021

Yet another iteration of this is included in #2412 (I lost track of how many PRs i had made to try to fix this in the past), which is stable now, so I think this PR may no longer be needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sago35 @niaow @aykevl @deadprogram