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

merykitty's attempt #114

Merged
merged 10 commits into from
Jan 6, 2024
Merged

merykitty's attempt #114

merged 10 commits into from
Jan 6, 2024

Conversation

merykitty
Copy link
Contributor

@merykitty merykitty commented Jan 4, 2024

My attempt at this challenge, test.sh merykitty passed.

Please kindly review, thanks very much.

@gunnarmorling
Copy link
Owner

Oh, wow, this looks very promising. 7.379s on the eval env, fastest so far! Summoning the current top of the pack @royvanrijn, @ebarlas, @spullara, for feedback too!

@merykitty
Copy link
Contributor Author

It turns out that just prefetching the file manually helps lower the time by 20% on my machine, now it takes around 2 seconds, for comparison the baseline takes around 106 seconds and just reading the whole file without doing anything takes around 0.75 seconds.

Thanks.

Copy link
Owner

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

Very nice! This comes in at 7.6, i.e. first place currently! I've added some questions in line to further my (and others) understanding. It looks alright to me, but getting some more intuition would be great. Thanks a lot!

}

private static long iterate(PoorManMap aggrMap, MemorySegment data, long offset) {
var line = ByteVector.fromMemorySegment(BYTE_SPECIES, data, offset, ByteOrder.nativeOrder());
Copy link
Owner

Choose a reason for hiding this comment

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

I suppose this would fail if station names are e.g. 100 double-byte characters, right? Not a problem per se, we agreed yesterday on enforcing a 100 bytes limit for station names (rather than 100 UTF-8 characters), I'm just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No if we cannot find the delimiter which means the key is longer than the segment we fall back to slow path which do so in a more straightforward way,

Copy link
Owner

Choose a reason for hiding this comment

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

Ah yes, got it. Thanks for adding all the comments!

return offset;
}

private static long iterate(PoorManMap aggrMap, MemorySegment data, long offset) {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you provide a description of how that iteration loop works? I roughly get it, but it's quite dense code and having a high-level description would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

long designMask = ~(signed & 0xFF);
long digits = ((word & designMask) << shift) & 0x0F000F0F00L;

// Now digits is in the form 0xUU00TTHH00
Copy link
Owner

Choose a reason for hiding this comment

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

What is TT, HH, etc? As for the main loop, could you provide an overview of how this thing works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UU stands for the value of the units digit, TT is the tens digit and HH is the hundreds digit.

@merykitty
Copy link
Contributor Author

After trying again without prefetching I am also able to reach 2s, which is weird.

Thanks for your reviews, hope I have addressed those.

@gunnarmorling gunnarmorling merged commit 4fc6034 into gunnarmorling:main Jan 6, 2024
@gunnarmorling
Copy link
Owner

00:07.620, current #1! Very nice, thanks for participating!

@DamienOReilly
Copy link

How does this one fare our with Graal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants