You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by GitBox <gi...@apache.org> on 2020/09/21 16:48:55 UTC
[GitHub] [lucenenet] theolivenbaum opened a new pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
theolivenbaum opened a new pull request #344:
URL: https://github.com/apache/lucenenet/pull/344
Minor change, but noticed this was appearing on my benchmark when trying to enumerate all search hits - don't think the dictionary is actually needed here
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #344:
URL: https://github.com/apache/lucenenet/pull/344#issuecomment-696558106
Hi @NightOwl888 , good point regarding the comments, will keep it in mind for future commits!
One thing that comes to mind, what's the official goal reg. version parity with the Java Lucene source-code? Besides keeping binary compatibility with 4.8, is the goal to evolve exactly as in the original source-code / binary protocol, or is LuceneNet planning to diverge from it? Just to understand what kind of code changes could be introduced here :)
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 merged pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
Posted by GitBox <gi...@apache.org>.
NightOwl888 merged pull request #344:
URL: https://github.com/apache/lucenenet/pull/344
----------------------------------------------------------------
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
[GitHub] [lucenenet] theolivenbaum commented on pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
Posted by GitBox <gi...@apache.org>.
theolivenbaum commented on pull request #344:
URL: https://github.com/apache/lucenenet/pull/344#issuecomment-696558106
Hi @NightOwl888 , good point regarding the comments, will keep it in mind for future commits!
One thing that comes to mind, what's the official goal reg. version parity with the Java Lucene source-code? Besides keeping binary compatibility with 4.8, is the goal to evolve exactly as in the original source-code / binary protocol, or is LuceneNet planning to diverge from it? Just to understand what kind of code changes could be introduced here :)
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 merged pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
Posted by GitBox <gi...@apache.org>.
NightOwl888 merged pull request #344:
URL: https://github.com/apache/lucenenet/pull/344
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #344:
URL: https://github.com/apache/lucenenet/pull/344#issuecomment-696585044
> One thing that comes to mind, what's the official goal reg. version parity with the Java Lucene source-code? Besides keeping binary compatibility with 4.8, is the goal to evolve exactly as in the original source-code / binary protocol, or is LuceneNet planning to diverge from it? Just to understand what kind of code changes could be introduced here :)
Our goal is to keep enough parity with Java so when it comes time to upgrade we can usually map a type and its members in Java to its counterpart in .NET. We try to separate what is essentially the application design from the language features of Java. In general, we don't want to diverge too much from the original design unless it is to use a .NET convention instead of a Java convention, some examples:
* Converting Java iterators to .NET enumerators
* Naming conventions (Java `.size()` to .NET `Count`, for example)
* Java uses `begin` and exclusive `end` index, .NET uses `startIndex`, `length`
* Java tends to nest public types inside of each other and in .NET public types are in namespaces (except in collections)
* Java tends to use abstract classes and leverage the language feature of anonymous classes to implement methods inline, in .NET it is generally better to make constructors with delegate parameters to pass the method implementations and not make the class abstract
So in general, a type in .NET should represent a similar type in Java with a similar name and similar purpose. As long as the type has similar members and has the same purpose it did in Java, we can modify its internal workings to make it more efficient, if needed.
That being said, if a part of the public API design is inefficient in .NET, doesn't follow a common .NET practice, or can be adapted to make it more usable (such as enabling LINQ support or type initialization support), it warrants further discussion. For the time being, we hold development discussions on the [dev mailing list](https://lucenenet.apache.org/contributing/index.html#start-a-discussion).
BTW - It appears that the `Cache` type only needed to be non-generic to make it fit into the collection so it can be enumerated over. I am in the process of changing it now, but we can now remove a lot of casting and [some boxing/unboxing](https://github.com/apache/lucenenet/blob/de5251c848b79223d394778cbb17ffc1388877ba/src/Lucene.Net/Search/FieldCacheImpl.cs#L1773) from `FieldCacheImpl` by making `Cache` generic and getting rid of the loops that iterate over the fields and just call the individual member variables directly.
Speaking of which, if you find any other places where boxing/unboxing is a problem, please open an issue and/or submit a PR to fix it. We ended up with some boxing and quite a bit of extra casting when converting from Java, and it is worth a look to see if we can remove it and still have all of the tests pass, if possible.
----------------------------------------------------------------
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
[GitHub] [lucenenet] NightOwl888 commented on pull request #344: remove unnecessary dictionary lookup in FieldCacheImpl
Posted by GitBox <gi...@apache.org>.
NightOwl888 commented on pull request #344:
URL: https://github.com/apache/lucenenet/pull/344#issuecomment-696585044
> One thing that comes to mind, what's the official goal reg. version parity with the Java Lucene source-code? Besides keeping binary compatibility with 4.8, is the goal to evolve exactly as in the original source-code / binary protocol, or is LuceneNet planning to diverge from it? Just to understand what kind of code changes could be introduced here :)
Our goal is to keep enough parity with Java so when it comes time to upgrade we can usually map a type and its members in Java to its counterpart in .NET. We try to separate what is essentially the application design from the language features of Java. In general, we don't want to diverge too much from the original design unless it is to use a .NET convention instead of a Java convention, some examples:
* Converting Java iterators to .NET enumerators
* Naming conventions (Java `.size()` to .NET `Count`, for example)
* Java uses `begin` and exclusive `end` index, .NET uses `startIndex`, `length`
* Java tends to nest public types inside of each other and in .NET public types are in namespaces (except in collections)
* Java tends to use abstract classes and leverage the language feature of anonymous classes to implement methods inline, in .NET it is generally better to make constructors with delegate parameters to pass the method implementations and not make the class abstract
So in general, a type in .NET should represent a similar type in Java with a similar name and similar purpose. As long as the type has similar members and has the same purpose it did in Java, we can modify its internal workings to make it more efficient, if needed.
That being said, if a part of the public API design is inefficient in .NET, doesn't follow a common .NET practice, or can be adapted to make it more usable (such as enabling LINQ support or type initialization support), it warrants further discussion. For the time being, we hold development discussions on the [dev mailing list](https://lucenenet.apache.org/contributing/index.html#start-a-discussion).
BTW - It appears that the `Cache` type only needed to be non-generic to make it fit into the collection so it can be enumerated over. I am in the process of changing it now, but we can now remove a lot of casting and [some boxing/unboxing](https://github.com/apache/lucenenet/blob/de5251c848b79223d394778cbb17ffc1388877ba/src/Lucene.Net/Search/FieldCacheImpl.cs#L1773) from `FieldCacheImpl` by making `Cache` generic and getting rid of the loops that iterate over the fields and just call the individual member variables directly.
Speaking of which, if you find any other places where boxing/unboxing is a problem, please open an issue and/or submit a PR to fix it. We ended up with some boxing and quite a bit of extra casting when converting from Java, and it is worth a look to see if we can remove it and still have all of the tests pass, if possible.
----------------------------------------------------------------
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