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

Renamed makeUnbufferedConnection and used it for VuMeter controls #4583

Merged
merged 3 commits into from
Jan 9, 2022

Conversation

JoergAtGithub
Copy link
Member

This is the Follow-UP of #4566 which replaces makeConnection for VuMeter/L/R controls by makeCompressedConnection in controller the mappings that comes with Mixxx.
These controls appear in short intervals and are primary responsible for event queue overload on slow computers. On the other hand, these are numeric values, which are no used in the application logic of the mappings. Therefore the mapping logic can't be influenced by skipping superseded values.
This PR doesn't replace the use of legacy ConnectControl statements, because in theses cases it wouldn't be a 1:1 replacement without risk.

@daschuer
Copy link
Member

pre-commit fails

@daschuer
Copy link
Member

The code looks good.

@JoergAtGithub
Copy link
Member Author

pre-commit fails

Yes, but how can I fix: "Executable eslint not found"

Maybe a dummy commit to rerun the CI Actions?

@daschuer
Copy link
Member

We can rerun them from the GitHub GUI.

@daschuer
Copy link
Member

Just did it.

@daschuer
Copy link
Member

I don't think the commit: Debug vcpkg_copy_pdbs should be added.

Yes, this was just for debugging.

@daschuer
Copy link
Member

I see in the log:
[INFO] Installing environment for https://github.com/pre-commit/mirrors-eslint.
and
Executable `eslint` not found

@Holzhaus Do you have an idea why this fails?

@JoergAtGithub
Do you have the eslint hook installed and succeeds it locally?
I think we can merge anyway in this case.

@JoergAtGithub
Copy link
Member Author

No unfortunately not - I'm on Windows, where local pre-commit isn't available.

I see an eslint update in #4558 a month ago. Maybe eslint was never executed since then, and nobody noticed?

@Holzhaus
Copy link
Member

No unfortunately not - I'm on Windows, where local pre-commit isn't available.

Why do you think that? I'm pretty certain it works on Windows, too.

@JoergAtGithub
Copy link
Member Author

It's written in the Wiki, that it's Linux only. And I also tried myself, it just hangs and no commit is possible. Also on CI, it's just executed on Linux.

@Swiftb0y
Copy link
Member

The frontpage of the pre-commit website mentions multiple times that hooks were tested on Linux, macOS and Windows, so I can't believe it doesn't work on there.

@Holzhaus
Copy link
Member

It's written in the Wiki, that it's Linux only.

No idea who added that, I didn't.

And I also tried myself, it just hangs and no commit is possible.

Weird, we should definitely debug this. Does it set up the environment or does that also hang?

Also on CI, it's just executed on Linux.

There is no point of running pre commit on all 3 OSes because the output should be exactly the same. It would just be a waste of electricity.

@JoergAtGithub JoergAtGithub force-pushed the Use_makeCompressedConnection branch from 93e9c5d to ee35919 Compare January 1, 2022 10:19
@JoergAtGithub JoergAtGithub marked this pull request as draft January 1, 2022 11:52
@JoergAtGithub
Copy link
Member Author

@Swiftb0y I did an experimental commit, that reverted the changes from #4580 and now eslint is found on GitHub again.

@Swiftb0y
Copy link
Member

Swiftb0y commented Jan 1, 2022

mhmmm. I'm still confused, this shouldn't be happening. I mean pre-commit won't pass for any of the modified mappings anyways so we can ignore that. Still, I'm confused why its broken on CI. I guess I'll try to reproduce the issue in a VM when I find the time. I really did not mean to break CI.

@Holzhaus
Copy link
Member

Holzhaus commented Jan 1, 2022

mhmmm. I'm still confused, this shouldn't be happening. I mean pre-commit won't pass for any of the modified mappings anyways so we can ignore that. Still, I'm confused why its broken on CI. I guess I'll try to reproduce the issue in a VM when I find the time. I really did not mean to break CI.

I suppose you overwrote the additional_dependencies list and replaced it with a list containing just eslint-jsdoc, but not eslint itself?

@uklotzde
Copy link
Contributor

uklotzde commented Jan 1, 2022

mhmmm. I'm still confused, this shouldn't be happening. I mean pre-commit won't pass for any of the modified mappings anyways so we can ignore that. Still, I'm confused why its broken on CI. I guess I'll try to reproduce the issue in a VM when I find the time. I really did not mean to break CI.

I suppose you overwrote the additional_dependencies list and replaced it with a list containing just eslint-jsdoc, but not eslint itself?

#4595

@JoergAtGithub JoergAtGithub force-pushed the Use_makeCompressedConnection branch from 7d1214d to c70b873 Compare January 2, 2022 00:16
@JoergAtGithub JoergAtGithub marked this pull request as ready for review January 2, 2022 00:19
@JoergAtGithub
Copy link
Member Author

Uwes fix worked! eslint is fount again.

Now eslint fails as expected - I think these unrelated eslint changes shouldn't be fixed in this PR?

@Holzhaus
Copy link
Member

Holzhaus commented Jan 2, 2022

Now eslint fails as expected - I think these unrelated eslint changes shouldn't be fixed in this PR?

No, don't worry about that 😅

A bit OT: as someone who didn't follow the original PR #4566 I really had problems to understand what a "compressed" connection is. I think the name is misleading because we don't try to reduce the file size, we just drop obsolete packages. I think we should consider changing the name (but I won't insist on that). If we do, we should do that before merging this to avoid unnecessary commits in the git history.

IIRC we do the opposite of tail drop policy often used in network equipment, i.e. we drop old packages instead of new ones, correct?

How about these:

  • makeConnectionWithHeadDropPolicy
  • makeFlushingConnection
  • makeCongestionControlledConnection
  • makeDeduplicatingConnection

@JoergAtGithub
Copy link
Member Author

makeFlushingConnection sounds goog to me, but I'm not sure if this is the correct English word for this. In other parts of the code, I use SkipSuperseded, but this is very long.

@uklotzde
Copy link
Contributor

uklotzde commented Jan 2, 2022

How about makeUnbufferedConnection()? From the client's perspective values that are not consumed in time are discarded because no internal buffering is provided.

@JoergAtGithub
Copy link
Member Author

Makes sense to me, we don't need to expose the technical implementation details to the user.

@JoergAtGithub JoergAtGithub changed the title Use makeCompressedConnection for VuMeter controls Renamed makeUnbufferedConnection and used it for VuMeter controls Jan 3, 2022
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@Holzhaus Holzhaus merged commit e5a00a7 into mixxxdj:main Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants