-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Preliminary support for ARM #25318
Preliminary support for ARM #25318
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments.
core/licenses/jna-4.4.0.jar.sha1
Outdated
d85e9ceee4d97e738524a231687ff28446376c97 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this change? How is the SHA changing without the version? And I think we shouldn't change the version unless upstream has incremented the version and we are incrementing with them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SHA changes because there are more bits (the aarch64-related files). The version doesn't change because it's apparently tied to the upstream JNA version, which is still 4.4.0
AFAICT. If we can bump the version that would definitely be preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understand that, but we can not re-release 4.4.0 so we can not change the SHA without changing the version as is done here. I would prefer that we not bump the version until upstream bumps, and they are still on 4.4.0. We can consider releasing a 4.4.0-1 or some other moniker to differentiate it from 4.4.0 if upstream does not bump soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually prefer the extension to permanently differentiate (like an OS package), but this kind of change is so infrequent maybe it's not worth it. Maybe I should take it up on elastic/jna-build#1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we differentiate based on the group ID, I don't think we need more than that as this need release out of sync should be infrequent as you say indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like an OS package
I was thinking along those lines too....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've implemented this as part of elastic/jna-build#1 in elastic/jna-build@b6661ab. Please review that particular change over there. I'll bump the version and SHA here.
@@ -243,6 +243,8 @@ static SockFilter BPF_JUMP(int code, int k, int jt, int jf) { | |||
Map<String,Arch> m = new HashMap<>(); | |||
m.put("amd64", new Arch(0xC000003E, 0x3FFFFFFF, 57, 58, 59, 322, 317)); | |||
m.put("i386", new Arch(0x40000003, 0xFFFFFFFF, 2, 190, 11, 358, 354)); | |||
// 64-bit only! | |||
m.put("aarch64", new Arch(0xC00000B7, 0xFFFFFFFF, 1079, 1071, 221, 281, 277)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These make sense. I haven't checked the system call numbers but I trust that they are right. I will double check before final review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I checked https://github.com/torvalds/linux/blob/9705596d08ac87c18aee32cc97f2783b7d14624e/include/uapi/asm-generic/unistd.h and they all look right to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the comment to "arm is 64 bit only" so it doesn't get confused with the line above it supporting 32 bit intel.
@@ -269,7 +269,7 @@ public void testSearchWithMatrixStats() throws IOException { | |||
assertEquals(5, matrixStats.getFieldCount("num2")); | |||
assertEquals(29d, matrixStats.getMean("num2"), 0d); | |||
assertEquals(330d, matrixStats.getVariance("num2"), 0d); | |||
assertEquals(-0.13568039346585542, matrixStats.getSkewness("num2"), 0d); | |||
assertEquals(-0.13568039346585542, matrixStats.getSkewness("num2"), 1.0e-16); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's happening here? If this is correct now for x64, and needs to be changed for ARM (why?) how is this not going to turn around and break on x64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For whatever reason only getSkewness()
has less precision on aarch64
:
amd64: -0.13568039346585542
aarch64: -0.1356803934658554
It still passes on Intel, because the delta is still valid. It makes the assertion slightly more permissive, not less. I scratched my head over this one as well, curious why only this assertion needed a delta. I asked @jpountz about it. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's puzzling!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, I misread the assertion at first but the puzzle why a change is needed only here remains! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is puzzling indeed. It could be a bug, but it could also be due to differences in the JVM/hardware that causes the documents to be stored in a different order in the index. Then at search time, aggs collect documents in a different order as well and we get different rounding errors?
The build check failed with this error message:
|
This is because you're pulling the stock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
* master: (129 commits) Add doc note regarding explicit publish host Fix typo in name of test Add additional test for sequence-number recovery WrapperQueryBuilder should also rewrite the parsed query. Remove dead code and stale Javadoc Update defaults in documentation (#25483) [DOCS] Add docs-dir to Painless (#25482) Add concurrent deprecation logger test [DOCS] Update shared attributes for Elasticsearch (#25479) Use LRU set to reduce repeat deprecation messages Add NioTransport threads to thread name checks (#25477) Add shortcut for AbstractQueryBuilder.parseInnerQueryBuilder to QueryShardContext Prevent channel enqueue after selector close (#25478) Fix Java 9 compilation issue Remove unregistered `transport.netty.*` settings (#25476) Handle ping correctly in NioTransport (#25462) Tests: Remove platform specific assertion in NioSocketChannelTests Remove QueryParseContext from parsing QueryBuilders (#25448) Promote replica on the highest version node (#25277) test: added not null assertion ...
This commit adds preliminary support for 64-bit ARM architectures. Relates #25318
Thanks @drewr. |
Do you have any guides to install elasticsearch on a aarch64 ubuntu server or a docker image for aarch64 ? |
These commits produce a working Elasticsearch binary and get the full test suite to pass. It has not been tested in any production capacity. It depends on elastic/jna-build#1, which adds
aarch64
support to our custom JNA jar.Development system is currently an ARMv8, 64-core, Huawei Hi1616 Type 2a2 from our friends at Packet.