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

Fix AST disk cache bugs #396

Open
wants to merge 6 commits into
base: release-0.5.x
Choose a base branch
from

Conversation

vic-cw
Copy link

@vic-cw vic-cw commented Jun 4, 2019

No description provided.

Previously, if disk AST cache was effectively used, incremental
compilation would crash when checking for asynchronous calls.

This commit fixes it.
Previously, when AST disk cache was effectively used, incremental
compilation would crash. This was due to a wrong deserialization
of variables.

This commit fixes it.
Previously, if AST disk cache was effectively used, incremental
compilation would crash. This was due to a logic error in
unary and binary expression deserialization.

This commit fixes it.
Previously, if AST disk cache was effectively used, incremental
compilation would crash. This was due to a logic error in
qualified expression deserialization.

This commit fixes it.
Previously, even if incremental option was set on TeaVMTool, AST
disk cache was not used.

This commit fixes it.
@konsoletyper
Copy link
Owner

What is the purpose of patching 0.5.x?

@vic-cw
Copy link
Author

vic-cw commented Jun 5, 2019

Hi Alexey,

Thanks for the swift reply.

I am confused by your question. I don't see any answer I could give you that wouldn't be an obvious software engineering lesson. Thus I am thinking I might be missing something. Would you care to explain?

On another note, I felt like I am a disturbance when reading your message, quite the opposite of feeling welcome. Are you happy that people propose bug fixes and improvements to TeaVM? Or would you rather be the sole contributor?

@konsoletyper
Copy link
Owner

I suppose most of the bugs related to AST cache are fixed by now in master. I don't maintain 0.5.x branch for a long time, so I just can't review the code, sorry. Anyway, what is the reason to patch 0.5.x, while you can use (and contribute to) 0.6.0, which is more stable, robust and fast?

@vic-cw
Copy link
Author

vic-cw commented Jun 6, 2019

Thanks for the swift reply.

The technical point

I still don't understand what the misunderstanding is here, since the story seems to be stereotypical of software usage and releases, but since it seems the way forward, here it is:

  • I want to make my build faster, because AST disk cache is not used due to bugs
  • I built my app using 0.5.1
  • I tried upgrading to the latest build on bintray, but compiler crashed. I guess it is a mix of regressions and breaking changes. Both are normal for commits in between releases, this is why the concept of releases exists
  • I don't want to rewrite all of my app right now just to handle breaking changes

Yet I don't see that it hurts having a release with bugs fixed. I actually believe that this is a crucial thing to have in order for a piece of software to be used widely.

Let me know if I got this wrong, but it seems to me that 0.5.1 is the latest release. I haven't seen any 0.6.0 tag, nor release, or even a release branch. Thus, in addition to forcing me to bear the high cost of rewriting part of my app, using a dev build would have the following stereotypical disadvantages for me:

  • My app would depend on a dev build, not a release version
  • It is likely that I would run into bugs. The problem is that in order to get the fix for these bugs, I would need to upgrade to another dev version. That new dev version would be likely to have received new commits with new features in the meantime, likely to include regressions. Thus I might go on for a very long time not having something stable.

Did you take a look at the code for these changes? Most of them are very mathematical, so it's easy to tell they are correct by just reading them, and they're just a few lines.

The human question

I am very disturbed by the fact that you totally ignored my comments and questions on your former answer's human aspect.

I told you that it made me feel bad, and while it could have been a misunderstanding, the fact that you ignored my comment seems to tell that it wasn't.

I don't know how to interpret your silence other than confirming that I am a disturbance to you, and that you don't deem me worthy of respect enough to receive an answer.

I would love to be wrong, and I would love that you clear this further misunderstanding. In the current state of affairs I feel disrespected.

@konsoletyper
Copy link
Owner

As for technical aspects. You are right, however, you should understand that the only contributor to the project is me, and I have only very small amount of time to maintain it, whereas TeaVM is really big. So I'm kind of indisciplined regarding release cycle questions, etc, etc. If only I had a chance to do TeaVM fulltime and perhaps hire people, I would do this aspect a little better. Another reason for being indisciplined is the fact that I'm looking for a chance to make TeaVM popular, and I'm trying it in different directions (for now, I succesfully compiled https://edu.cospaces.io/), so I simply have no straight plan. I agree that a good piece of software needs a good and clear release cycle and rodmap, but this in turn requires time, which I can get if I proove that TeaVM is a good software. You see?

As for 0.6.0 release. I don't do major thing regarding JS anymore, in fact I can release 0.6.0 right now, but I was too careless to make Flavour part of TeaVM project, so to make a new release I have to either upgrade Flavour to latest TeaVM state or to make a hard decision to drop Flavour at all. That's the only fact that does not allow to release 0.6.0. So I believe it's safe to start using dev builds right now and ask me to fix remaining issues.

I'm not going to puslish more 0.5.x releases anymore and I even don't remember what's going on in 0.5.x branch, so how are you expecting me to review this?

As for human aspects. What offended you in my initial comment which was

What is the purpose of patching 0.5.x?

I don't say any bad words there. I just meant what I said: I was interested what was the purpose. I was surprised you were offended by these words. Anyway, I don't think github is an appropriate place to make such discussions.

Please, look at this from my side. I have my thoughts where and how to improve TeaVM, so when you publish PR, I may be disagree. So it's a good practice to discuss with me first.

@vic-cw
Copy link
Author

vic-cw commented Jun 7, 2019

Thanks for the detailed reply. I appreciate your taking the time to clear things out.

About discussing

I have absolutely no problem with discussing the relevance of a pull request, or debating, or that you wouldn't agree from the start, or that you wouldn't agree at all. This is absolutely normal, is sound practice, and I am 100% OK with that.

As I'll explain below, I don't even really care that this gets merged.

On the human question

What made me feel unwelcome was not the fact that you asked a question, but the following:

  • the format of your first reply
    • no greeting
    • no gratitude
    • no acknowledgement that this might have been significant work on my end, especially getting set up to develop TeaVM for the first time, and tackling directly core issues
    • no softness on the question
  • the fact that your second reply seemed to ignore my concerns on this human question

This might be just a format issue, and it might be that in reality, you meant to be very welcoming. But the above reasons made me feel the opposite. To avoid a misunderstanding I asked you about it. The fact that I didn't get a response worsened it.

On the technical matter

As I mentioned, I don't really care whether this gets merged or not. I can build my project on my machine with my changes. It is only a pity that other people won't benefit from them.

It is your call, and if you don't want to merge them, that's fine.

Personally I think I would lean towards merging obvious bug fixes like this one, but again that's your call.

Regarding cutting a release branch:

  • I wouldn't want that my production application refer to a Bintray repository
  • I am using Flavour, so based on what you are saying I am under the impression that this wouldn't work right now
  • I have a few improvements in mind that I would like to contribute to TeaVM before the next release. I hope I can get them out before you cut that release branch.

@konsoletyper
Copy link
Owner

What made me feel unwelcome was not the fact that you asked a question, but the following:

  • the format of your first reply
    • no greeting
    • no gratitude
    • no acknowledgement that this might have been significant work on my end, especially getting set up to develop TeaVM for the first time, and tackling directly core issues
    • no softness on the question
  • the fact that your second reply seemed to ignore my concerns on this human question

Ah, I see now. I do think these 'greetings' and 'gratitude' are just waste of time. When computers communicate over some protocols, say HTTP, then usually don't tell 'please' or 'goodbye' to each other (unless this is needed by practical needs to, say, determine when actual payload ends). I wasn't taught to be a diplomat, but rather to be an engineer. And most of engineers I've ever communicated share my thoughts regarding this (they don't feel offended by not using some of these 'social protocol' words), so it's quite usual to me to not to say all these 'please' words discussing pure technical questions. I just try not to be toxic, and this is usually enough. Another problem is the lack of time and language barrier. I really sorry that this my approach made you uneasy.

To avoid a misunderstanding I asked you about it. The fact that I didn't get a response worsened it.

Ok, I just did not undestand what the concern was. Just tried to make technical apects clear and thought it was enough.

It is your call, and if you don't want to merge them, that's fine.

I didn't say I don't want to merge them. I was just

  • Interested what is the purpose of patching 0.5.x version
  • Provided my reasons no to do this.

It is only a pity that other people won't benefit from them.

Yes, it's a pity. On another hand, it's a pity for people not having a new 0.6.0 release. And for project like TeaVM, which does not have a big community or a business behind it, term 'release' is quite vague. When I engineer sofrware for my employer, we have distinct road map, we know what and when we are to release (because we have managers to do that), we can test release before publishing (because we have QA team), we have feedback from our community (because we have community) which helps us to prioritize issues for new releases. In case of one-man project without large community behind it, I can publish whatever I want and call it "release", so it really does not matter whether to use artifacts from Maven central, bintray or from your private repository with your custom builds.

Moreover, when I worked at Kolin team, our primary users were JetBrains internal users developing our internal products. They usually sat on the bleeding edge (nightly builds), despite we had released versions. The motivation was: until they released their own project, then can build against whatever they want. And sitting on the bleeding edge was the best opportunity to have shortest round-trip (they reported issues for their recent needs and we could deliver fixes in a very short term). So I only can propose this: sit on the bleeding edge, provide feedback to me and I'll be able to make TeaVM more stable and to release in shortly (by the time you'll release your project). IMO, extra discipline is usually a bad practice (although I agree that in most cases discipline is a good thing and indiscipline is a bad thing).

I wouldn't want that my production application refer to a Bintray repository

So I explained my thoughts regarding this point above.

I am using Flavour, so based on what you are saying I am under the impression that this wouldn't work right now

I think Flavour would work in a very primitive way, i.e. when you build it from Maven. However, there's new thing called development server, and due to different reasons, Flavour is incompatible with this, by design. So I just wanted to introduce some minor breaking changes to Flavour (like a new annotation to mark serializable objects and the requirement to put it on every class that is serializable by Flavour). BTW, developer's experience, including recompilation speed, was my focus during last few moths, so I believe most of the cache issues in 0.5.x are fixed in master.

I have a few improvements in mind that I would like to contribute to TeaVM before the next release.

You are welcome! I'll be happy to get your contribution, however I'd like you to discuss these improvements with me first.

@vic-cw
Copy link
Author

vic-cw commented Jun 12, 2019

Thanks a lot for the swift, detailed, and caring reply. I appreciate a lot having cleared the misunderstandings, and I am happy to be able to collaborate in these conditions.

On human matters

Thanks for explaining your point of view. It is very helpful for me.

I still believe that we are not computers, you included, and that being nice to each other isn't a waste of time, it is a booster of energy.

While some people might be happier with a strictly efficient communication, I believe most people are not. Here are some links that might be interesting reads:

Again, while I disagree with what you said, I appreciate that you explained your point of view, and that you said you were sorry that this made me uneasy. This goes a long way.

On technical matters

  • I personally feel like one crucial factor for TeaVM to take off is not a lack of features, but rather its stability. I would tend to think that if there were formal "releases" with bug fixes or non-breaking changes, it would probably be used more widely. Publishing something as a release is not only about calling it a release but about providing the assurance that this version will be maintained with bug fixes without introducing regressions or breaking changes so that people can rely on it.
  • The project I am using TeaVM on is a Saas project, hence I already have a release. In fact, I release several times per day.
  • Another reason why I am not keen on depending on a Bintray repository is that I am using my Gradle plugin to build, and I also want it to be clean. In order to use Bintray builds through my Gradle plugin, I would need to have a branch of it pointing to a Bintray build, and that user code would point to that branch of the Gradle plugin. It starts to be really messy.

Previously, when flushing AST disk cache, regular methods were
written to the same files as if they were async methods. As a
consequence, when trying to read these cached files, they would
always be missing. Effectively AST disk cache was not providing
any speed for regular methods, but slowing things down by
uselessly writing to disk.

This commit fixes it by getting cache to write to correct file
name.
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.

2 participants