Skip to content

[skiplang/skc] Re-enable clang inlining. #744

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

Draft
wants to merge 4,040 commits into
base: main
Choose a base branch
from

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Feb 11, 2025

Inlining in clang was disabled because the inlining heuristics had changed in clang 15, leading to compile time blowups. The situation seems to have been fixed in clang 19 (possibly in an earlier version, we need to check with clang 16 once #736 is merged).

Compile times for stage 2 skc:

  • Before this commit:
    clang 15, without inlining: 160.805s
  • After this commit:
    clang 15, with inlining: 600.047s
    clang 19, with inlining: 173.062s

mbouaziz and others added 30 commits January 15, 2025 10:51
WebAssembly.instantiate like it as much as Uint8Array
Saves a conversion
simplifies code
With initial docs on code formatting and git pre-commit hook.
With initial docs on code formatting and git pre-commit hook.
bennostein and others added 29 commits February 5, 2025 16:04
Some basic package READMEs. which will then render in the browser on
npmjs.com for our packages.

close SkipLabs#712
This takes all of our Skip runtime packages to 0.0.10, increments all of
our core `@skip-wasm/*` and `@skiplang/*` packages (some of which are
already at 1.0.x, and none of which users will install, so I left out of
the everything-to-0.0.10 edict), and stops versioning the test
package.json.
Temporary disconnect hackernews lib check
- Internal packages are managed with symbolic links.
When calling `runWithGc()` without a `synchronizer`, the global lock
was released after each iteration, and never re-acquired.
When calling `runWithGc()` without a `synchronizer`, the global lock was
released after each iteration, and never re-acquired.

The code path did not seem to be actively used, as it would
systematically crash.
Increase buffer times to de-flake the expectation tests on our examples,
leaving enough time for servers to spin up fully before clients start
hitting them.

This passed 20x in a row locally just now, but circleCI seems more
inconsistent in general so testing up there as well. Marking as draft
for now as I expect this may require a couple iterations.
This PR adds a git submodule for the docs_site repo and adjusts the docs
workflow to update its contents, removing the need to manually sync the
generated files between repos. The workflow for publishing the docs to
the
docs site is now (from www/README.md):
```
- $ make docs-publish
```
which will suggest to run:
```
Test locally: make docs-serve
Push to live site: cd www/docs_site/; git add -A; git commit -m 'update to <commit>'; git push; cd -
```
Once the suggested command ending in `git push` is executed, the live
docs
site will update in a few seconds.
Before this PR the following reliably fails for me:
```
$ npm install && npm run build
$ git clean -Xdf
$ npm install && npm run build
```
In the final step, the build sees the symlinks created in the first step
that were deleted in the second step as some sort of zombie, where
`fs.exists` thinks they do not exist but `fs.symlink` fails because they
do. Checking in a shell shows that `ls` is similarly confused.

This PR changes the build from checking if the link exists and only if not
calling symlink to creating a symlink with a temporary name and then
renaming it to the desired name. The temporary names are randomly generated,
and POSIX requires the rename to be atomic.
Before this PR the following reliably fails for me:
```
$ npm install && npm run build
$ git clean -Xdf
$ npm install && npm run build
```
In the final step, the build sees the symlinks created in the first step
that were deleted in the second step as some sort of zombie, where
`fs.exists` thinks they do not exist but `fs.symlink` fails because they
do. Checking in a shell shows that `ls` is similarly confused.

This PR changes the build from checking if the link exists and only if
not
calling symlink to creating a symlink with a temporary name and then
renaming it to the desired name. The temporary names are randomly
generated,
and POSIX requires the rename to be atomic.
Inlining in clang was disabled because the inlining heuristics had
changed in clang 15, leading to compile time blowups. The situation
seems to have been fixed in clang 19 (possibly in an earlier version).

Compile times for stage 2 skc:
- Before this commit:
    clang 15, without inlining: 160.805s
- After this commit:
    clang 15, with inlining: 600.047s
    clang 19, with inlining: 173.062s
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.

8 participants