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

qdel optimize #3558

Merged
merged 17 commits into from
Oct 22, 2024
Merged

qdel optimize #3558

merged 17 commits into from
Oct 22, 2024

Conversation

FeenieRU
Copy link
Contributor

@FeenieRU FeenieRU commented Oct 14, 2024 •
edited
Loading

About The Pull Request

That PR optimizes qdel and SSgarbage procs.

Based on:
tgstation/tgstation#79568
tgstation/tgstation#76956
tgstation/tgstation#80443
tgstation/tgstation#80628

Why It's Good For The Game

Better performance. Tested on downstream: CeladonSS13#1025

Changelog

🆑
code: Changing qdel() and SSgarbage procs
code: rewrite /Destroy(force, silent) to /Destroy(force)
/:cl:

@FeenieRU FeenieRU requested a review from a team as a code owner October 14, 2024 14:37
@github-actions github-actions bot added DME Edit Code change Watch something violently break. labels Oct 14, 2024
@FeenieRU
Copy link
Contributor Author

FeenieRU commented Oct 14, 2024 •
edited
Loading

Main problem for compiling - no libdreamluau.so per compiling

tools/ci/install_dreamluau.sh
tools/tgs_scripts/PreCompile.sh

@thgvr
Copy link
Member

thgvr commented Oct 14, 2024

A better solution than adding another auxtool modification maintained by one person we don't even have contact with would be actually just fixing harddels

MarkSuckerberg
MarkSuckerberg requested changes Oct 14, 2024
View reviewed changes
Copy link
Member

@MarkSuckerberg MarkSuckerberg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Alright, so: this PR is alright, but you misunderstand what you ported here. Dreamluau doesn't change garbage collection/qdel at all, except for cleaning up its own references in the luau code. All the optimizations besides it that are in this PR is fine, but Dreamluau 100% does not need to be included here, there is absolutely no benefit it gives us, in fact, the only thing it could possibly do is slow down GC, even if by a trivial amount, because again, it only cleans up its own references, it doesn't touch the DM code at all.

Basically, this PR is in dire need of atomization. You can port Dreamluau in a different PR if you'd like, but this PR will not be merged with both qdel optimizations and a full port of an entirely unused external library.

Also, please make sure to credit the original TG PRs that added these changes, if you could please.

code/controllers/subsystem/garbage.dm Outdated Show resolved Hide resolved
@FeenieRU
Copy link
Contributor Author

Done. Sorry, I'm low-sorted coder.

MarkSuckerberg
MarkSuckerberg approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@MarkSuckerberg MarkSuckerberg left a comment

Choose a reason for hiding this comment

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

Understandable, and it's looking a lot better. Don't forget code/__HELPERS/auxtools.dm before it's merged, and I'll see if we can get this testmerged soon.

@github-actions github-actions bot removed the DME Edit label Oct 14, 2024
@FeenieRU
Copy link
Contributor Author

Don't forget code/__HELPERS/auxtools.dm

Done

@thgvr thgvr added the Test Merge Required They'll have to do it for free. label Oct 14, 2024
@FeenieRU
Copy link
Contributor Author

изображение

Whyyyy?! How much I should update it?

@Erikafox
Copy link
Contributor

image
I assume the create and destroy test is failing on the same spot each time

@FeenieRU
Copy link
Contributor Author

Any ideas for fix?

@FalloutFalcon
Copy link
Contributor

FalloutFalcon commented Oct 15, 2024 •
edited
Loading

I don't see any unit tests ran on there and its a drafted pr it really doesn't seem like its been "tested" there.
skimming the code I don't see anything that's clearly more performant though it looks like it should return earlier in some situations. What are you basing that off of?

Better performance. Tested on downstream: CeladonSS13#1025

This also ports parts of
tgstation/tgstation#80443
tgstation/tgstation#76956
tgstation/tgstation#80628
but not everything which is my initial guess to why copy pasting the code didn't work

@MarkSuckerberg MarkSuckerberg mentioned this pull request Oct 15, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Oct 16, 2024
@MarkSuckerberg
More Harddel Fixes (#3569)
ef3efbe 
<!-- Write **BELOW** The Headers and **ABOVE** The comments else it may
not be viewable. -->
<!-- You can view Contributing.MD for a detailed description of the pull
request process. -->

## About The Pull Request
Fixes a few harddels that surfaced in PR #3558 

## Why It's Good For The Game
Harddels are still just terrible wastes of time

## Changelog

:cl:
/:cl:

<!-- Both :cl:'s are required for the changelog to work! You can put
your name to the right of the first :cl: if you want to overwrite your
GitHub username as author ingame. -->
<!-- You can use multiple of the same prefix (they're only used for the
icon ingame) and delete the unneeded ones. Despite some of the tags,
changelogs should generally represent how a player might be affected by
the changes rather than a summary of the PR's contents. -->
@FeenieRU
Copy link
Contributor Author

I don't see any unit tests ran on there and its a drafted pr it really doesn't seem like its been "tested" there. skimming the code I don't see anything that's clearly more performant though it looks like it should return earlier in some situations. What are you basing that off of?

Better performance. Tested on downstream: CeladonSS13#1025

This also ports parts of tgstation/tgstation#80443 tgstation/tgstation#76956 tgstation/tgstation#80628 but not everything which is my initial guess to why copy pasting the code didn't work

Done

FalloutFalcon
FalloutFalcon approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@FalloutFalcon FalloutFalcon left a comment

Choose a reason for hiding this comment

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

Looks good I will give it a test merge today or this weekend, only small thing is the existing harddel but ill fix it if you dont.

@FalloutFalcon
Copy link
Contributor

and and update your change log

@FeenieRU
Copy link
Contributor Author

and and update your change log

ready

@FeenieRU
Copy link
Contributor Author

Looks good I will give it a test merge today or this weekend, only small thing is the existing harddel but ill fix it if you dont.

honestly, idk why it happens

@MarkSuckerberg
Copy link
Member

I fixed the harddel in #3572, this is fine to TM, ideally with my PR though

@FeenieRU
Copy link
Contributor Author

everything is okay?

FeenieRU pushed a commit to FeenieRU/Shiptest-Feenie that referenced this pull request Oct 20, 2024
@MarkSuckerberg @FeenieRU
More Harddel Fixes (shiptest-ss13#3569)
52c3c43 
<!-- Write **BELOW** The Headers and **ABOVE** The comments else it may
not be viewable. -->
<!-- You can view Contributing.MD for a detailed description of the pull
request process. -->

## About The Pull Request
Fixes a few harddels that surfaced in PR shiptest-ss13#3558 

## Why It's Good For The Game
Harddels are still just terrible wastes of time

## Changelog

:cl:
/:cl:

<!-- Both :cl:'s are required for the changelog to work! You can put
your name to the right of the first :cl: if you want to overwrite your
GitHub username as author ingame. -->
<!-- You can use multiple of the same prefix (they're only used for the
icon ingame) and delete the unneeded ones. Despite some of the tags,
changelogs should generally represent how a player might be affected by
the changes rather than a summary of the PR's contents. -->
@FeenieRU
Copy link
Contributor Author

изображение

are you kidding me...

@FeenieRU
Copy link
Contributor Author

FINALLY

@FalloutFalcon FalloutFalcon added this pull request to the merge queue Oct 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2024
@FeenieRU
Copy link
Contributor Author

what's up with docker?..

@FalloutFalcon FalloutFalcon added this pull request to the merge queue Oct 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2024
@Sun-Soaked Sun-Soaked added this pull request to the merge queue Oct 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 22, 2024
@Sun-Soaked
Copy link
Contributor

what's up with docker?..

I made a mistake clearing up mine references, give me a moment to post an emergency pr

@Sun-Soaked Sun-Soaked added this pull request to the merge queue Oct 22, 2024
Merged via the queue into shiptest-ss13:master with commit eb94f92 Oct 22, 2024
14 checks passed
shiptest-server pushed a commit that referenced this pull request Oct 22, 2024
@actions-user
Automatic changelog generation for PR #3558 [ci skip]
68d118a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkSuckerberg MarkSuckerberg MarkSuckerberg approved these changes

@FalloutFalcon FalloutFalcon FalloutFalcon approved these changes

Assignees
No one assigned
Labels
Code change Watch something violently break. Test Merge Required They'll have to do it for free.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@FeenieRU @thgvr @Erikafox @FalloutFalcon @MarkSuckerberg @Sun-Soaked