Comments (5)
Some other thoughts: the PR that triggered this was huge. It involved many things including the changes mentioned above. Perhaps a better approach would have been submitting these as separate PRs so that each feature change could be considered as a whole, and then merged in after discussion. As it was, no one reviewed it because it was too big.
Here is, I think, my new rule: I do not plan on accepting any new code that I do not fully understand myself. This will prevent work that may be beneficial but complicated. I'm quite alright with having it be a bit slower if it is easier to reason about what is going on. The race condition bug from before is an exceedingly strange bug that appeared to take a long time to diagnose and fix. I'm willing to pay the speed price for the benefit of not having bugs like that be possible.
from goon.
I have to admit I was surprised you pulled the serialization code in given the known issues with it that @xStrom detailed.
I too only quickly reviewed it; it was quite large. I didn't see any issues, but I didn't try to understand every piece of it either; just at a high level. Combined with being busy on other projects at the moment it didn't get my full attention.
I was really surprised by the metrics/results. I wouldn't have thought the standard gob encoder was that slow. It wasn't clear to me why the new solution was actually faster, but again, I didn't really dig into it.
from goon.
Increasing the complexity of the code base is a slippery slope that needs to be treaded carefully. If it were only the data migration features at the cost of the complexity, then I would agree that it's not worth it. However the performance gains aren't marginal. As evidenced by my benchmarking, deserializing simple entities has a 10x (!!) speedup with this more complex version. The performance difference is even greater in long-lived instances. Additionally, it's possible to tweak this complex version even more for gains, which isn't really possible with a simple gob.Encode
call. I think the added complexity is worth it for this case, primarily due to the performance gains, which can be massive.
That said, I am completely willing to work to make this compex version more understandable. There could be some low hanging refactoring fruit, which I could look for. For a bigger effect though, I could greatly increase the number of comments. I originally kept the comments minimal because the existing goon code had few comments, and over-verbose comments can make code tedious to read. However a better balance of verbosity/obscurity could be found by adding some comments to the current version.
Additionally, perhaps we should introduce an architecture document for internal development, where we describe some inner workings & design decisions. Basically this would be a place where I could describe at a high level how the complex serialization engine works and why it's so much faster than encoding/gob
. Spoiler: basically encoding/gob
is optimized for serializing 100MB, not 10KB.
Regarding huge pull requests, I completely agree that pull requests should be smaller rather than bigger. However for this case it wasn't really possible to do a smaller changeset. The pull request could have been reduced in size by not having any of the performance enhancements in it, but then it wouldn't even meet my own merge requirements.
PS. Regarding goread breaking, are there still some unresolved issues with this new code?
from goon.
@xStrom No, there are no currently-known issues with the code and goread.
I'll read this later tonight, but putting this here for my memory: if it is decided to remove the code, we should consider using appengine/memcache.Gob as a replacement.
from goon.
I misread some of those benchmark results. I didn't realize the performance gain was as high as 10x, I thought it was somewhat less than twice as fast. Yes, since the gain is that high I think it's worth keeping this. I'm not thrilled that we got hit with that race condition bug since it suggests bugs of a similar class could be out there and they are nasty.
If you feel like either documenting those functions or producing some longer multi-paragraph comment somewhere about the serialization methods, I'm not opposed. It's still some hundreds of lines of pretty dense code, so I think it may, even then, be unlikely that I sit down and read through it all carefully. Do what you want. Thanks.
from goon.
Related Issues (20)
- Adding a property to an entity HOT 1
- Expose Underlying Context in Goon type? HOT 4
- laterGet and laterPut HOT 5
- Consistency issue using aetest HOT 10
- Documentation appstats usage HOT 2
- Adding a Put to memory only call HOT 3
- Add support for google.golang.org/appengine packages HOT 18
- Can code depending on goon be unit tested without devserver? HOT 2
- Changes by PropertyLoadSaver.Load aren't cached. HOT 5
- Using context.Context in place of *http.Request HOT 5
- Use FromGoon in docs
- What's license? HOT 2
- TestEmbeddedStruct fails HOT 2
- GetMulti and Get don't ignore field mismatch in Tx HOT 2
- Failed to decode field config - extra data in buffer HOT 3
- Make compatible with cloud.google.com/go HOT 9
- Why the name `goon`? HOT 2
- Reusing entity cause slice values to be appended HOT 2
- Request: Add release tag for go modules HOT 3
- Surface a way to know the final cache key of entities. HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from goon.