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.

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

Lsc info fix #3627

Closed
wants to merge 14 commits into from
Closed

Lsc info fix #3627

wants to merge 14 commits into from

Conversation

ah-OOG-ah
Copy link
Member

Replace the linked lists of boxed longs with a simple array.

@Dream-Master Dream-Master requested a review from a team December 9, 2024 19:28
@Dream-Master Dream-Master added the bug fix Fix a bug. Please link it in the PR. label Dec 9, 2024
@Georggi
Copy link

Georggi commented Dec 9, 2024

Works on our server (Zvezdolet, cherry-picked on top of .103 tag). LSC no longer lags.

@Glease
Copy link
Contributor

Glease commented Dec 10, 2024

wouldn't this make hour long average a constant 0 if IO is below 72000EU/t? This would leads to unusable low precision at IV~early LuV IMO.

@Georggi
Copy link

Georggi commented Dec 10, 2024

wouldn't this make hour long average a constant 0 if IO is below 72000EU/t? This would leads to unusable low precision at IV~early LuV IMO.

No, it doesn't do that, here is an example of a base which had that level.
image
There is a problem which we've discussed, where for the first hour (after server restart for instance) the values are not precise, because division is by constant number of samples, instead of number of samples actually received
https://discord.com/channels/181078474394566657/603348502637969419/1315770149386846218

@combusterf
Copy link
Contributor

wouldn't this make hour long average a constant 0 if IO is below 72000EU/t? This would leads to unusable low precision at IV~early LuV IMO.

It would look like it works, if energy comes and goes in in batches over the EU/t limit (e.g. laser hatches would do that), but I don't see it work for the actual scenario.

The fix is easy though: don't do the division where the += and -= are at, but instead at the getter.

@Georggi
Copy link

Georggi commented Dec 10, 2024

Yeah, nvm, looking at the code for the second time, it probably underreports due to that. Even though it seems correct.

@Georggi
Copy link

Georggi commented Dec 10, 2024

There is a potential long overflow problem (which I think exists in 2.7 as well), where if you UXV 16M laser hatch, which is 9*10^15, and multiply that by 72000 (max times you can sum), which if awfully close to 2^63)

@ah-OOG-ah
Copy link
Member Author

There is a potential long overflow problem (which I think exists in 2.7 as well), where if you UXV 16M laser hatch, which is 9*10^15, and multiply that by 72000 (max times you can sum), which if awfully close to 2^63)

There shouldn't be any long over/underflow issues unless the EU input is greater than LONG_MAX in a single tick, as the average is identical to what you'd get if you took an average normally - it can't overflow unless there's at least one data point which exceeds LONG_MAX, because even if every datapoint was just under LONG_MAX, dividing N of those by N and then adding them back up isn't going to give you a result higher than LONG_MAX

wouldn't this make hour long average a constant 0 if IO is below 72000EU/t? This would leads to unusable low precision at IV~early LuV IMO.

This is almost certainly an issue though. I believe the fix would just be to use a double to represent it until double precision becomes a problem (which is 2**53 iirc).

@ah-OOG-ah
Copy link
Member Author

wouldn't this make hour long average a constant 0 if IO is below 72000EU/t? This would leads to unusable low precision at IV~early LuV IMO.

It would look like it works, if energy comes and goes in in batches over the EU/t limit (e.g. laser hatches would do that), but I don't see it work for the actual scenario.

The fix is easy though: don't do the division where the += and -= are at, but instead at the getter.

This would work, but unlike using a double to accumulate it would have the overflow problem Georggi mentioned.

@Georggi
Copy link

Georggi commented Dec 10, 2024

There shouldn't be any long over/underflow issues unless the EU input is greater than LONG_MAX in a single tick, as the average is identical to what you'd get if you took an average normally

Sorry I haven't made myself clear. Yeah, in current implementation, because you divide right away, it's not a problem. If the division were to be done when getting the value, but if it was done in the getter, as combusterf suggested, it would be close enough to maybe be a problem.

Another case though (just for consideration). I think there can be a case with UMV (or a couple of UIV capacitors) it breaks a bit? outputLastTick and inputLastTick are already longs, so it's probably not for this PR (if at all needed), but the data there might be incorrect (lower than actual) and it's such an edge case that it probably should be ignored.

Comment on lines +111 to +112
private final long[] energyInput = new long[BUFFER_LEN];
private final long[] energyOutput = new long[BUFFER_LEN];
Copy link
Member

Choose a reason for hiding this comment

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

These two arrays are very large, do we need a full hour for precision or can we get away with less?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original code reached back one hour. I don't know what it was used for and so didn't attempt to change what it did, I just attempted to make the existing code better.

Comment on lines +115 to +116
private final double[] averageIOd = new double[6];
private final long[] averageIOl = new long[6];
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to track both long and double, instead of just double, if the extra size here is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The original rationale was because doubles lose precision above 2**53 - however, running the numbers finds that even at LONG_MAX doubles are more accurate.

The only problem with doubles is that they have weird floating errors. I might redo this to store two long trackers instead - one that divides after summing for higher precision, and one that divides before for when the former overflows (when avg. input exceeds LONG_MAX / NUM_SAMPLES per tick).

sum += l;
}
return sum / Math.max(energyInputValues.size(), 1);
return averageIOl[0] > pow(2, 53) ? averageIOl[0] : (long) averageIOd[0];
Copy link
Member

Choose a reason for hiding this comment

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

What is the significance of pow(2, 53)? Could that be extracted to a constant and given a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably because that is max precise value storable by a double

Copy link
Contributor

Choose a reason for hiding this comment

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

Still pow should be avoided as much as possible for a simple power of 2 calculation. at least keep the result at a static final field. Math.pow() do the full iterative pow() algorithm and is bad for performance

@Dream-Master Dream-Master requested a review from a team December 14, 2024 14:44
@Glease
Copy link
Contributor

Glease commented Dec 15, 2024

alternative implementation at https://github.com/GTNewHorizons/GT5-Unofficial/tree/fix/lsc-127. this one similarly uses a long ring buffer to record the past values, but instead uses 2 long fields per value to track the sum using 128 bit integer arithemtic (actually 127bit, but it hardly matters). the real average value is caculated when it is actually needed by BigInteger.divide(). no double is involved

@Dream-Master Dream-Master deleted the lsc-info-fix branch January 8, 2025 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Fix a bug. Please link it in the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants