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

Limit the number of reported warnings #6577

Merged
merged 10 commits into from
May 10, 2023

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented May 5, 2023

Pull Request Description

Artifically limiting the number of reported warnings to 100. Also added benchmarks with random Ints to investigate perf issues when dealing with warnings (future task).
Ideally we would have a custom set-like collection that allows us internally to specify a maximal number of elements. But EnsoHashMap (and potentially EnsoSet) are still WIP when it comes to being PE-friendly.

The change also allows for checking if the limit for the number of reported warnings has been reached. It will visualize by adding an additional "Warnings limit reached." to the visualization.

The limit is configurable via --warnings-limit parameter to run.

Closes #6283.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.

@radeusgd
Copy link
Member

radeusgd commented May 5, 2023

This seems like just a wrong solution to this problem.

We will be losing warnings that may be important through this. Imagine that a Vector.sort returns 1000 warnings about comparison but some other operation returns 1 very important warning about an encoding issue. With this, we will get the first 100 warnings all being the not so helpful Vector.sort issue and the encoding warning will become swallowed.

The idea of the ticket #6283 was to limit reported warnings for Vector.sort specifically. I think every function we have that runs risk of reporting too many warnings should have some way of its own to limit this number. An automatic solution is just very prone to data loss.

If we still want to limit the number of warnings globally, let's just ensure we attach an artificial warning which will indicate that additional warnings were hidden - so that the user will at least know that there are some hidden warnings - without it we are losing data that the user should see.

@radeusgd
Copy link
Member

radeusgd commented May 5, 2023

The idea of the ticket #6283 was to limit reported warnings for Vector.sort specifically. I think every function we have that runs risk of reporting too many warnings should have some way of its own to limit this number. An automatic solution is just very prone to data loss.

And indeed, it may be desirable to have some semi-automatic way to do this, as probably many methods on Vector could do this - like map or filter.

So maybe some global limit could be needed after all. But I still think the Vector.sort should have its specific limit (otherwise it will tend to uselessly saturate the global limit and any other warnings will always be swallowed - the issue is that Vector.sort can very often enter a state where it will emit LOTS of warnings, for stuff like map it should be a bit rarer).

But still, I think we absolutely have to have an indicator that 'there are more warnings which are hidden'.

@hubertp
Copy link
Contributor Author

hubertp commented May 5, 2023

Yes, we can have "data loss" but do you imagine people scrolling through 100 warnings because 101th was "the important one" ?
It is similar to what compiler is doing; it is not reporting more than X warnings/errors because at some point it becomes just impossible to digest.

@radeusgd
Copy link
Member

radeusgd commented May 5, 2023

Yes, we can have "data loss" but do you imagine people scrolling through 100 warnings because 101th was "the important one" ? It is similar to what compiler is doing; it is not reporting more than X warnings/errors because at some point it becomes just impossible to digest.

Fair. But we should indicate that there are more warnings that got swallowed - so that the user know that there is something more.

And ideally, IMO we should limit the amount of warnings coming from the Vector.sort separately, to some lower value like even first 10. Otherwise a single Vector.sort will saturate all warnings and nothing else will show up in nodes that depend on that result.

@hubertp
Copy link
Contributor Author

hubertp commented May 5, 2023

Fair. But we should indicate that there are more warnings that got swallowed - so that the user know that there is something more.

That can be added, yes.

And ideally, IMO we should limit the amount of warnings coming from the Vector.sort separately, to some lower value like even first 10. Otherwise a single Vector.sort will saturate all warnings and nothing else will show up in nodes that depend on that result.

Can be added as well.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

I'd like us to eliminate @TruffleBoundary one day...

@JaroslavTulach
Copy link
Member

JaroslavTulach commented May 5, 2023

We will be losing warnings that may be important through this.

That's completely OK. Nobody will scan more than 100 warnings unless really, really interested in. javac also limits number of errors to 100 by default and I haven't yet see a developer to complain. Having an option to tune the number of kept warnings would allow users to re-run the application and see more warnings. Although the right approach is to eliminate the warnings, I guess.

we should indicate that there are more warnings that got swallowed

+1 one to that.

@@ -28,27 +32,50 @@
@OutputTimeUnit(TimeUnit.MILLISECONDS)
Copy link
Member

Choose a reason for hiding this comment

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

Is the warmup sufficient? I have a minor suggestion. You don't have to do this as I plan to do that in the future for all the benchmarks, but it would help with the stability of these benchmarks.

Modify the context to include an option for -D.engine.TraceCompilationDetails=true and check whether the output contains any "[engine]" logs, i.e., whether there are any Graal/Truffle compilations going on in the measurement phase. I guess it is enough to redirect the output to the stdout and just look when did the warmup phase ended and whether there are some logs from Graal. If you struggle with that too much, don't do that.

Copy link
Member

Choose a reason for hiding this comment

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

An inspiration how to do that easily is here: #6271 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I missed that comment.

@hubertp
Copy link
Contributor Author

hubertp commented May 9, 2023

Screenshot from 2023-05-09 13-24-57

Artifically limiting the number of reported warnings to 100.
Also added benchmarks with random ints to investigate perf issues when
dealing with warnings (future task).

Closes #6283.
Warnings library now allows for checking if a maximal number of warnings
has been reported. Currently that number is fixed. Making it
configurable is the job for the next change.
User can pass `--warnings-limit <arg>` to limit the number of warnings
reported. This however requires the context to be passed around, to be
able to retrieve runtime option.
@hubertp hubertp force-pushed the wip/hubert/6283-limit-reported-warnings branch from b787c49 to 429ff1f Compare May 9, 2023 15:52
Otherwise `reached_max_count` was reporting invalid result.
Used different variations which was confusing.
@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label May 10, 2023
@mergify mergify bot merged commit cf3624c into develop May 10, 2023
@mergify mergify bot deleted the wip/hubert/6283-limit-reported-warnings branch May 10, 2023 11:48
Procrat added a commit that referenced this pull request May 10, 2023
* develop:
  Limit the number of reported warnings (#6577)
  Add COOP+COEP+CORP headers (#6597)
  Fix issues with missing sourcemaps (#6572)
  Fix asset delete; implement project delete and project rename (#6566)
  Fix #6377: Change ctrl-r shortcut (#6620)
Procrat added a commit that referenced this pull request May 11, 2023
…ing-6287

* develop:
  Fix shortcuts table formatting (#6644)
  Automatic type based dropdown does not include singleton in a union type (#6629)
  Make Meta.get_annotation work for constructor (#6633)
  Limit the number of reported warnings (#6577)
  Add COOP+COEP+CORP headers (#6597)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vector.sort should attach limited number of warnings
6 participants