Closed sqonk closed 2 years ago
@sqonk Thanks for submitting this. For the future, it would help if you could spend a few lines of text in your PR description on the purpose of/motivation for the changes you're proposing.
@DennisdeBest On my machine, these changes bump the performance of your solution from approx. 220 iterations to around 410. Would you see any reason not to merge this?
@rbergen Just looking under the posted description now and am surprised to see it has nothing at all between the title and the requirements! My apologies for this, I had typed out about 3 small paragraphs under the bracketed section when I posted the PR, not sure how it got lost. Would you like me to re-post my description to the to a comment?
@sqonk I just checked the PR description markdown, and your lines are indeed there. They just weren't showing in the rendered description. Maybe it's because they followed an empty comment tag? Technically that shouldn't have this result, but it could have confused the GitHub markdown renderer. In any case, I removed the comment tag, and now your text does show. Which is exactly what I was looking for. So, thanks for including it in the beginning. :)
EDIT: I now see you edited your description two minutes before I did. Which means I don't know what was where when. It doesn't really matter, either.
@rbergen No I think it was me all along. I'd gone and put the original text in the actual markup comment, hence why it never showed. I investigated and moved it down, which was the cause of your confusion. My apologies.
@sqonk That would indeed explain what we weren't seeing without the markdown renderer misbehaving. In response to what we saw yesterday, I have already changed the PR template to not include the comment tags in the Description section. That should avoid this type of confusion in the future.
I think we can now just wait for the response of @DennisdeBest. I'll give them a few more days to give one before proceeding on this.
@rbergen yes not a worry. I am actually quite new to the whole PR thing on Github. In hindsight I should have probably picked up that it was a comment but I had most of my focus on the contributing guidelines and making sure there was nothing I had missed.
Assuming there are no issues, I look forward to seeing the benchmarks in any case. I clocked around 930 passes on my own laptop (up from the mid 400s to memory), and hit about 1040 with the docker build.
I tried a bunch of different approaches to boost the score but admittedly the SPLFixedArray is actually reasonably good at a task like this. On the other hand, Strings in PHP are a simple byte array behind the scenes and are technically not immutable to the same degree as some other languages, as I understand it, so modifying the string with array syntax for individual chars, should essentially be changing the 0s and 1s directly.
At the moment that is about as close to the horses mouth as I think I can get it without resorting to external dependancies.
Hello, sorry I didn't reply earlier, thanks @sqonk it's very interesting to see these changes ! Thanks @rbergen for merging this.
Description
Changing SPLFixedArray to a simple string yields a noticeable performance increase. In local tests - up to a factor of 2x.
I have also updated the Docker build to the use the main official PHP image running the latest version at time of writing (8.1.7). V8.1 has its own minor speed increases over the previous 8.0. Particularly, the Opcache JIT is now supported on ARM64.
Contributing requirements
drag-race
as the target branch.