Skip to content

Remove runtime.GC() workaround resolved in Go 1.12#1090

Open
loqimean wants to merge 2 commits into
go-spatial:masterfrom
loqimean:remove-runtime-gc-hack
Open

Remove runtime.GC() workaround resolved in Go 1.12#1090
loqimean wants to merge 2 commits into
go-spatial:masterfrom
loqimean:remove-runtime-gc-hack

Conversation

@loqimean

@loqimean loqimean commented Apr 7, 2026

Copy link
Copy Markdown
  • remove explicit runtime.GC() and stale workaround comment
  • keep seeding behavior intact on modern Go GC
  • add seedWorker tests for success, no-overwrite cache hit, and cache miss seeding
  • test run: go test ./cmd/tegola/cmd/cache -run TestSeedWorker

The manual runtime.GC() call after atlas.SeedMapTile was a workaround
for golang/go#14045 (large arrays not being garbage collected). That
issue was fixed in Go 1.12; the module now requires Go 1.26.1, so the
hack and its TODO comment are no longer needed.

Also adds TestSeedWorkerSucceeds to cover the happy path of seedWorker.
@loqimean loqimean requested review from ARolek and gdey as code owners April 7, 2026 12:18
@iwpnd

iwpnd commented Apr 7, 2026

Copy link
Copy Markdown
Member

Hmm.. that's an odd/meaningsless one and I suspect this to be a claude contribution. Not sure how you want to handle this @ARolek?

@ARolek

ARolek commented Apr 7, 2026

Copy link
Copy Markdown
Member

@iwpnd if we don't need to call the GC manually that would be ideal. This piece of code is so old, and I know Go's GC has improved dramatically from when this was implemented. I believe we were hitting OOM errors a long time ago.

I think this should be fine, but also won't know until it gets stress tested via some tile seeding.

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 48

Coverage increased (+0.2%) to 40.72%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 16476
Covered Lines: 6709
Line Coverage: 40.72%
Coverage Strength: 218.97 hits per line

💛 - Coveralls

@iwpnd

iwpnd commented Apr 7, 2026

Copy link
Copy Markdown
Member

I wasn't so much referring to the change itself - that's fine, although the tests are redundant. It's the blatantly obvious LLM signature that this PR has. Was wondering what you think of automated contributions like this.

@ARolek

ARolek commented Apr 7, 2026

Copy link
Copy Markdown
Member

@iwpnd

It's the blatantly obvious LLM signature that this PR has. Was wondering what you think of automated contributions like this.

This is a great question and something we should provide guidance on in this repo. I recently came across gdal's AI tool policy and I think it's pretty reasonable: https://gdal.org/en/stable/community/ai_tool_policy.html#ai-tool-policy.

I like their "golden rule"

Our golden rule is that a contribution should be worth more to the project than the time it takes to review it.

I think this applies to any PR, not just generated ones. Maybe this is worth opening up a discussion on this repo as I'm also interested in your thoughts.

@iwpnd

iwpnd commented Apr 8, 2026

Copy link
Copy Markdown
Member

I think a discussion would be a good idea when the outcome is a collective agreement that includes guidelines which we make publicly available to new contributors. 👍

@loqimean

loqimean commented Apr 9, 2026

Copy link
Copy Markdown
Author

I wasn't so much referring to the change itself - that's fine, although the tests are redundant. It's the blatantly obvious LLM signature that this PR has. Was wondering what you think of automated contributions like this.

hey, I added this tests by myself just to be sure that seeds are running fine, becuase it's a best practice to cover code with specs. I was surprised when I saw your comment, that it is "LLM contribution", it helped me just to write specs and I had a discussion of this change with another Go developer, who proved these specs and change itself.

@loqimean

loqimean commented Apr 9, 2026

Copy link
Copy Markdown
Author

yeah and I am not sure these specs are redundant, because there were no specs at all for the stuff I changed, so, I am not LLM when I covered my changes with specs, I am a developer who takes care about his changes in code by covering it with automated check (tests).

@iwpnd

iwpnd commented Apr 10, 2026

Copy link
Copy Markdown
Member

Im reviewing an ever growing number of LLM contributions at work, and even worse in my free-time on open source projects that are near and dear to me. This PR is comparable harmless to others, but shows the same pattern. But, I’m glad it will prime a discussion on how to handle LLM contributions in the future. We live in an age where people let a bunch of openclaw agents loose to karma farm contributions for their resume and as OSS contributor and maintainers, we have to align on how to handle this.

And with regards to the tests. I don’t mean meaningless as in their existence - there was indeed no tests for this branch yet. But they’re meaningless in that they do not cover the change that was made. This, and the use of old syntax while on the same hand claiming to modernize are a sure sign of the use of LLMs.

In the future I will address this differently as it’s never my intention to hurt or insult.

@loqimean

Copy link
Copy Markdown
Author

Im reviewing an ever growing number of LLM contributions at work, and even worse in my free-time on open source projects that are near and dear to me. This PR is comparable harmless to others, but shows the same pattern. But, I’m glad it will prime a discussion on how to handle LLM contributions in the future. We live in an age where people let a bunch of openclaw agents loose to karma farm contributions for their resume and as OSS contributor and maintainers, we have to align on how to handle this.

And with regards to the tests. I don’t mean meaningless as in their existence - there was indeed no tests for this branch yet. But they’re meaningless in that they do not cover the change that was made. This, and the use of old syntax while on the same hand claiming to modernize are a sure sign of the use of LLMs.

In the future I will address this differently as it’s never my intention to hurt or insult.

thank you for explanation, I really understand you, that a huge problem nowadays.
In my case modernization is in remove "TODO" with old way GB working, but really, LLM helped me to write specs to cover seed and to be sure nothing is broken, it may use old syntax, but and my profile language is ruby, so, if I can improve somehow this PR and you can recommend what to do in future, I would be very grateful.

And yeah, everything is important and even this small PR starts a great discussion about LLM in opensource, that's awesome, how small things can make big things 😄

@loqimean

Copy link
Copy Markdown
Author

@iwpnd @ARolek double ping

@loqimean

Copy link
Copy Markdown
Author

@iwpnd @ARolek double ping v2 😄

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.

4 participants