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: [email protected]
