Preallocate space for variables in run_context #1364

Merged

mfenniak merged 1 commit from steveno/runner:preallocate_execs into main 2026-02-14 20:01:19 +00:00

Conversation 12 Commits 1 Files changed 8 +14 -14

steveno commented 2026-02-09 19:43:09 +00:00

Contributor

Copy link

This should help with reducing allocations during runtime which,
in theory, could provide a performance boost. These changes were
recommended by the prealloc lint.

• other
  • PR: Preallocate space for variables in run_context

This should help with reducing allocations during runtime which, in theory, could provide a performance boost. These changes were recommended by the prealloc lint. <!--start release-notes-assistant--> <!--URL:https://code.forgejo.org/forgejo/runner--> - other - [PR](https://code.forgejo.org/forgejo/runner/pulls/1364): <!--number 1364 --><!--line 0 --><!--description UHJlYWxsb2NhdGUgc3BhY2UgZm9yIHZhcmlhYmxlcyBpbiBydW5fY29udGV4dA==-->Preallocate space for variables in run_context<!--description--> <!--end release-notes-assistant-->

steveno added 1 commit 2026-02-09 19:43:09 +00:00

Preallocate space for variables in run_context 1543ad3148

viceice reviewed 2026-02-09 20:06:14 +00:00

viceice left a comment

Owner

Copy link

can you please extend PR description what this improves?

can you please extend PR description what this improves?

steveno commented 2026-02-11 02:58:51 +00:00

Author

Contributor

Copy link

I re-enabled a lot of the linters provided by golangci-lint and realized I could correct a lot of the issues it highlighted.

I re-enabled a lot of the linters provided by golangci-lint and realized I could correct a lot of the issues it highlighted.

mfenniak commented 2026-02-11 03:30:25 +00:00

Owner

Copy link

@steveno wrote in #1364 (comment):

I re-enabled a lot of the linters provided by golangci-lint and realized I could correct a lot of the issues it highlighted.

Nice! Can you include enabling the related linter into this PR?

@steveno wrote in https://code.forgejo.org/forgejo/runner/pulls/1364#issuecomment-77769: > I re-enabled a lot of the linters provided by golangci-lint and realized I could correct a lot of the issues it highlighted. Nice! Can you include enabling the related linter into this PR?

steveno force-pushed preallocate_execs from 1543ad3148 to a02fe2b742 2026-02-12 05:25:32 +00:00 Compare

steveno commented 2026-02-12 05:32:30 +00:00

Author

Contributor

Copy link

Since the group seems receptive to the changes, I've added more to make this MR worth merging.

Can you include enabling the related linter into this PR?

I'd rather not for a few reasons. First, this MR doesn't correct all the issues for the reenabled linter and I don't want all the pipelines to start failing by default. Second, at least on my machine, the linter is acting a little goofy. For example, I updated act/runner/run_context.go:809 from 0 to len(rc.ServiceContainers) and now its spitting this at me - which is obviously wrong.

act/runner/run_context.go:809:3: Consider preallocating execs with capacity len(rc.ServiceContainers) + len(rc.ServiceContainers) (prealloc)
                execs := make([]common.Executor, len(rc.ServiceContainers))

Finally, @viceice asked me to update the PR description. Do you mean the commit message or the blurb here on forgejo?

Since the group seems receptive to the changes, I've added more to make this MR worth merging. > Can you include enabling the related linter into this PR? I'd rather not for a few reasons. First, this MR doesn't correct _all_ the issues for the reenabled linter and I don't want all the pipelines to start failing by default. Second, at least on my machine, the linter is acting a little goofy. For example, I updated `act/runner/run_context.go:809` from `0` to `len(rc.ServiceContainers)` and now its spitting this at me - which is obviously wrong. ``` act/runner/run_context.go:809:3: Consider preallocating execs with capacity len(rc.ServiceContainers) + len(rc.ServiceContainers) (prealloc) execs := make([]common.Executor, len(rc.ServiceContainers)) ``` Finally, @viceice asked me to update the PR description. Do you mean the commit message or the blurb here on forgejo?

viceice commented 2026-02-12 06:03:48 +00:00

Owner

Copy link

just a short description why this change is worth merging. it's not obvious from PR title/ commit message/ code change

just a short description why this change is worth merging. it's not obvious from PR title/ commit message/ code change

steveno force-pushed preallocate_execs from a02fe2b742 to 07bba2df35 2026-02-13 04:59:08 +00:00 Compare

steveno commented 2026-02-13 05:00:21 +00:00

Author

Contributor

Copy link

@viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed.

@viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed.

viceice commented 2026-02-13 05:57:45 +00:00

Owner

Copy link

@steveno wrote in #1364 (comment):

@viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed.

why didn't you added it to the PR description above as told? I've done it now. it makes review much easier because you can see the description much more prominent

@steveno wrote in https://code.forgejo.org/forgejo/runner/pulls/1364#issuecomment-77893: > @viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed. why didn't you added it to the PR description above as told? I've done it now. it makes review much easier because you can see the description much more prominent

mfenniak commented 2026-02-14 01:39:43 +00:00

Owner

Copy link

All of these preallocations are implemented slightly incorrectly, resulting in significant logic errors.

		names := make([]string, len(s.Runs))
		for _, r := range s.Runs {
 				names = append(names, r.JobID)
		}

This will create an array of length len(s.Runs), full of zero values. And then it will append new values to the end, resulting in an array that is 2*len(s.Runs) long.

What is intended here is names := make([]string, 0, len(s.Runs)), which creates an array of length 0, but capacity len(s.Runs) so that the append statement doesn't have to allocate memory.

All of these preallocations are implemented slightly incorrectly, resulting in significant logic errors. ```go names := make([]string, len(s.Runs)) for _, r := range s.Runs { names = append(names, r.JobID) } ``` This will create an array of **length** `len(s.Runs)`, full of zero values. And then it will append new values to the end, resulting in an array that is `2*len(s.Runs)` long. What is intended here is `names := make([]string, 0, len(s.Runs))`, which creates an array of length 0, but capacity `len(s.Runs)` so that the `append` statement doesn't have to allocate memory.

steveno commented 2026-02-14 18:57:31 +00:00

Author

Contributor

Copy link

@viceice wrote in #1364 (comment):

@steveno wrote in #1364 (comment):

@viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed.

why didn't you added it to the PR description above as told? I've done it now. it makes review much easier because you can see the description much more prominent

I'm sorry, I was a little confused. Next time I'll make sure both are filled out more fully.

@viceice wrote in https://code.forgejo.org/forgejo/runner/pulls/1364#issuecomment-77894: > @steveno wrote in #1364 (comment): > > > @viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed. > > why didn't you added it to the PR description above as told? I've done it now. it makes review much easier because you can see the description much more prominent I'm sorry, I was a little confused. Next time I'll make sure both are filled out more fully.

👍 1

steveno commented 2026-02-14 18:58:34 +00:00

Author

Contributor

Copy link

@mfenniak wrote in #1364 (comment):

All of these preallocations are implemented slightly incorrectly, resulting in significant logic errors.

This is a great catch! Did you catch this as part of the test suite, or did you just recognize I wrote them incorrectly?

@mfenniak wrote in https://code.forgejo.org/forgejo/runner/pulls/1364#issuecomment-77993: > All of these preallocations are implemented slightly incorrectly, resulting in significant logic errors. This is a great catch! Did you catch this as part of the test suite, or did you just recognize I wrote them incorrectly?

steveno force-pushed preallocate_execs from 07bba2df35 to 220bcdb865 2026-02-14 19:25:31 +00:00 Compare

mfenniak approved these changes 2026-02-14 20:01:03 +00:00

mfenniak left a comment

Owner

Copy link

Looks good to me, thanks for the contribution!

@steveno wrote in #1364 (comment):

This is a great catch! Did you catch this as part of the test suite, or did you just recognize I wrote them incorrectly?

Just code review. I'm pretty sure that many tests would have been broken, but I didn't trigger CI to run once I noted the problem. 🙂

Looks good to me, thanks for the contribution! @steveno wrote in https://code.forgejo.org/forgejo/runner/pulls/1364#issuecomment-78042: > This is a great catch! Did you catch this as part of the test suite, or did you just recognize I wrote them incorrectly? Just code review. I'm pretty sure that many tests would have been broken, but I didn't trigger CI to run once I noted the problem. 🙂

mfenniak merged commit 0eb080c791 into main 2026-02-14 20:01:19 +00:00

mfenniak referenced this pull request from a commit 2026-02-14 20:01:19 +00:00

chore: preallocate space for variables in run_context (#1364)

steveno deleted branch preallocate_execs 2026-02-14 20:33:45 +00:00

viceice commented 2026-02-14 21:16:13 +00:00

Owner

Copy link

@steveno wrote in #1364 (comment):

@viceice wrote in #1364 (comment):

@steveno wrote in #1364 (comment):

@viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed.

why didn't you added it to the PR description above as told? I've done it now. it makes review much easier because you can see the description much more prominent

I'm sorry, I was a little confused. Next time I'll make sure both are filled out more fully.

PR description is more important, because we mostly do squash merge, so only the PR description lands in default branch commit message. 🤗

@steveno wrote in https://code.forgejo.org/forgejo/runner/pulls/1364#issuecomment-78041: > @viceice wrote in #1364 (comment): > > > @steveno wrote in #1364 (comment): > > > @viceice I amended the git commit message and force pushed. Please let me know if further adjustments are needed. > > > > > > why didn't you added it to the PR description above as told? I've done it now. it makes review much easier because you can see the description much more prominent > > I'm sorry, I was a little confused. Next time I'll make sure both are filled out more fully. PR description is more important, because we mostly do squash merge, so only the PR description lands in default branch commit message. 🤗

viceice-bot referenced this pull request from actions/setup-forgejo 2026-02-19 01:36:13 +00:00

Update dependency forgejo/runner to v12.7.0 #896

mfenniak referenced this pull request from a commit 2026-02-19 03:00:44 +00:00

Update dependency forgejo/runner to v12.7.0 (#896)

forgejo-mirror referenced this pull request from a commit 2026-02-19 04:01:08 +00:00

Update module code.forgejo.org/forgejo/runner/v12 to v12.7.0 (forgejo) (#11363)

viceice-bot referenced this pull request 2026-02-20 20:19:16 +00:00

Update forgejo-runner to v12.7.0 #1394

Sign in to join this conversation.

Reviewers

No reviewers

mfenniak

Labels

Clear labels   

FreeBSD

  

Kind/Breaking

Breaking change that won't be backward compatible

  

Kind/Bug

Something is not working

  

Kind/Chore

It is no fun but it needs to be done, this is a chore.

  

Kind/DependencyUpdate

for pull requests that are created by renovate when updating a dependency

  

Kind/Documentation

Documentation changes

  

Kind/Enhancement

Improve existing functionality

  

Kind/Feature

New functionality

  

Kind/Security

This is security issue

  

Kind/Testing

Issue or pull request related to testing

  

Priority

Critical

The priority is critical

  

Priority

High

The priority is high

  

Priority

Low

The priority is low

  

Priority

Medium

The priority is medium

  

Reviewed

Confirmed

Issue has been confirmed

  

Reviewed

Duplicate

This issue or pull request already exists

  

Reviewed

Invalid

Invalid issue

  

Reviewed

Won't Fix

This issue won't be fixed

  

Status

Abandoned

Somebody has started to work on this but abandoned work

  

Status

Blocked

Something is blocking this issue or pull request

  

Status

Need More Info

Feedback is required to reproduce issue or to continue work

  

Windows

Windows ports is an independent project, see https://github.com/Crown0815/Forgejo-runner-windows-builder

  

linux-powerpc64le

powerpc64le

  

linux-riscv64

  

linux-s390x

  

run-end-to-end-tests

Run https://code.forgejo.org/forgejo/end-to-end with a runner compiled from this pull request

  

run-forgejo-tests

Run https://codeberg.org/forgejo/forgejo tests importing the runner from this pull request

No labels

FreeBSD

Kind/Breaking

Kind/Bug

Kind/Chore

Kind/DependencyUpdate

Kind/Documentation

Kind/Enhancement

Kind/Feature

Kind/Security

Kind/Testing

Priority

Critical

Priority

High

Priority

Low

Priority

Medium

Reviewed

Confirmed

Reviewed

Duplicate

Reviewed

Invalid

Reviewed

Won't Fix

Status

Abandoned

Status

Blocked

Status

Need More Info

Windows

linux-powerpc64le

linux-riscv64

linux-s390x

run-end-to-end-tests

run-forgejo-tests

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

3 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo/runner!1364

No description provided.