Skip to content

GH-118943: Fix a race condition when generating jit_stencils.h #118957

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

Merged
brandtbucher merged 5 commits into from
May 16, 2024
Merged

GH-118943: Fix a race condition when generating jit_stencils.h #118957

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3578ca6
Make the generation of jit_stencils.h atomic
brandtbucher May 11, 2024
bc32227
blurb add
brandtbucher May 11, 2024
dc9404c
Don't stick jit_stencils.h in a temporary directory
brandtbucher May 11, 2024
04ff733
Remove empty write
brandtbucher May 11, 2024
5266129
Feedback from code review
brandtbucher May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Don't stick jit_stencils.h in a temporary directory
  • Loading branch information
@brandtbucher
brandtbucher committed May 11, 2024
commit dc9404c5457a33da24f2564f60ae35d3fe9ae99a
31 changes: 16 additions & 15 deletions Tools/jit/_targets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,18 +180,18 @@ async def _compile(
await _llvm.run("clang", args_o, echo=self.verbose)
return await self._parse(o)

async def _build_stencils(
self, work: pathlib.Path
) -> dict[str, _stencils.StencilGroup]:
async def _build_stencils(self) -> dict[str, _stencils.StencilGroup]:
generated_cases = PYTHON_EXECUTOR_CASES_C_H.read_text()
opnames = sorted(re.findall(r"\n {8}case (\w+): \{\n", generated_cases))
tasks = []
async with asyncio.TaskGroup() as group:
coro = self._compile("trampoline", TOOLS_JIT / "trampoline.c", work)
tasks.append(group.create_task(coro, name="trampoline"))
for opname in opnames:
coro = self._compile(opname, TOOLS_JIT_TEMPLATE_C, work)
tasks.append(group.create_task(coro, name=opname))
with tempfile.TemporaryDirectory() as tempdir:
work = pathlib.Path(tempdir).resolve()
async with asyncio.TaskGroup() as group:
coro = self._compile("trampoline", TOOLS_JIT / "trampoline.c", work)
tasks.append(group.create_task(coro, name="trampoline"))
for opname in opnames:
coro = self._compile(opname, TOOLS_JIT_TEMPLATE_C, work)
tasks.append(group.create_task(coro, name=opname))
return {task.get_name(): task.result() for task in tasks}

def build(
Expand All @@ -211,18 +211,19 @@ def build(
and jit_stencils.read_text().startswith(digest)
):
return
with tempfile.TemporaryDirectory() as tempdir:
work = pathlib.Path(tempdir).resolve()
stencil_groups = asyncio.run(self._build_stencils(work))
new_jit_stencils = work / "jit_stencils.h"
with new_jit_stencils.open("w") as file:
stencil_groups = asyncio.run(self._build_stencils())
jit_stencils_new = out / "jit_stencils.h.new"
Comment thread
brandtbucher marked this conversation as resolved.
try:
with jit_stencils_new.open("w") as file:
file.write(digest)
if comment:
file.write(f"// {comment}\n\n")
file.write("")
for line in _writer.dump(stencil_groups):
file.write(f"{line}\n")
new_jit_stencils.replace(jit_stencils)
jit_stencils_new.replace(jit_stencils)
finally:
jit_stencils_new.unlink(True)
Copy link
Copy Markdown
Contributor

@hroncok hroncok May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to guess (and look up) what the boolean stands for. Would you mind using a keyword argument instead?

Suggested change
jit_stencils_new.unlink(True)
jit_stencils_new.unlink(missing_ok=True)

Copy link
Copy Markdown
Member Author

@brandtbucher brandtbucher May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good suggestion.



class _COFF(
Expand Down