lucenenet-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [lucenenet] NightOwl888 commented on pull request #345: Reduce casting
Date Tue, 22 Sep 2020 14:47:25 GMT

NightOwl888 commented on pull request #345:
URL: https://github.com/apache/lucenenet/pull/345#issuecomment-696770379


   > change (a != null && a is MyClass) to a is MyClass mc
   
   Careful, although VS2019 tries to get you to do this, I am fairly certain it reduces performance
(need some benchmarks to verify). The optimizations in Lucene are verbose, but they are often
more performant than the shorter syntax. I saw at least 1 place where the check for null first
before the check for the cast had a major impact on performance. 
   
   Also, if there are many cases where the cast `a is MyClass mc` will not pass, we are throwing
away an extra variable declaration that wouldn't be needed if it were inside of the `if` block.
Ideally, we would not allocate memory unless absolutely necessary.
   
   That said, `is null` instead of `== null` doesn't have a huge performance impact and is
more robust because it circumvents any `==` operator overload.
   
   > Also changed a few fields to read-only, and one regex variable to static as suggested
by the analyzer too.
   
   Usually this is the correct decision. Look out for `readonly` on `struct`, though - I have
seen it have a seriously negative impact on performance. Do note that Java doesn't have structs,
so we should also be looking for opportunities for them to improve performance.
   
   > Regarding your previous comment, do you think this kind of changes needs the "LUCENENET"
comments you mentioned?
   
   In general, if the change is due to a syntax or platform difference between .NET and Java,
the comment isn't strictly necessary. If the change actually reflects a change to Lucene's
design, then it should definitely be commented.
   
   > By the way, is there any docs or quick start on running the tests locally? And are
the tests run automatically on PR, or manually? Just to know if I should bother trying to
set it up locally, or easier to let them run and just check the results...
   
   The best way (although it takes longer) is to run it on Azure DevOps. You can set it up
on any free Azure DevOps account by following the instructions on the [README](https://github.com/apache/lucenenet/blob/master/README.md#building-and-testing).
   
   Due to permission issues, we cannot enable Azure DevOps to run on PRs, so they should be
run first before you submit the PR and we will run them again several times before approving
it.
   
   Careful, we don't usually want to make broad sweeping changes like this without thorough
testing, otherwise it could take days to figure out why some of the tests don't pass. The
tests depend directly on the asserts, so already this could lead to problems if it is not
fully verified.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



Mime
View raw message