Fixes #2259 - Boot from archive #2260
Conversation
|
I'm going to stat taking a look at this since it is related to something I've started working on. For future pull requests (no need to make a change to the ones you have pending) if you could try to keep them as independent as possible, it really makes it easier to review. For one thing shorter lists of changes are easier to review. But more importantly the same person might not be reviewing all of your pull requests, and so it may be held up waiting for review of the other portions. (I know that specifically in this case keeping the .editorconfig changes is useful). |
|
I know what you are talking about but as you noticed, I won't separate until you accept the The other thing is that I want my version (with all changes), but I can separate it and merge to my main if you accept the |
| get | ||
| { | ||
| for (int i = argumentCount; i > 0;) | ||
| yield return argumentStack[--i]; |
There was a problem hiding this comment.
I never liked yield return. Starting from the assumption that C# and .NET are two independent things makes it impossible to ever comprehend what yield does. I kept trying to figure out, "Every time I try to look up what yield does they keep redirecting that into explaining what the higher level concept of the .NET type IEnumerable does. Yeah I know what IEnumerable is, but I'm not asking about .NET stuff. I'm asking about a keyword, yield - that's the C# language spec, not the .NET framework spec. It's at a more low level than .NET. What did the keyword itself mean at a low level when you're not using it with IEnumerable?" It only became possible for me to understand the keyword when I finally had the epiphany that magic collusion between C# and .NET is indeed exactly how yield works. That yes, they did in fact put a keyword into the language itself that exists only to serve the needs of one type in the framework library. Not being able to see that as a possibility made it take a long time to understand yield. It just feels so ... wrong to do things that way.
There was a problem hiding this comment.
Also, @firda-cze , I looked through this PR's full list of changed files and I'm not seeing where you use these new IEnumerables that you added to Stack.cs. You said you had them in here because you needed to use them elsewhere but where are you using them?
(Specifically I mean these:
Arguments
ArgumentsFromBottom
Scopes
ScopesFromBottom
)
There was a problem hiding this comment.
Well, you can create full blown enumerator, but this is just easier. You write the main loop and yield return elements that you can consume through foreach later, a bit like async, but without a thread. Co-routines in unity are implemented the same way and they actually use it in thread-like (fiber) fashion.
See https://github.com/firda-cze/KOS/blob/firda/src/kOS.Tester/TesterCPU.cs for the reason I added these. Definitelly using Arguments, wanted to use ArgumentsFromBottom for checking what has changed, before I understood the argument order reversing in call. I did not create view for Scopes yet, but will need that too.
Here #2247 are pictures from the tester.
There was a problem hiding this comment.
Well, I disagree with it being easier, but that's a matter of opinion. (In trying to find the sweet spot between being hard because it's too terse versus being hard because it's too verbose, my opinion is that the C# culture errs on the side of being too terse. The C# community loves syntactic sugar a lot more than I do.)
I understand how much of a PITA it is to keep things separate in one PR that you're using in another PR when neither is merged yet. @hvacengi is dealing with the other related merge so I'll defer to him to decide how to handle it.
By the way, is that tester something you plan to release? Is it an independent executable or something that you invoke inside KSP's process?
There was a problem hiding this comment.
It is independent exe, only referencing the Safe.dll, feel free to take it any time, open-source, free to use, but it is not yet ready, it lacks the scopes.
But it can already step individual instructions, show opcodes, all jumps/calls, stack, output, log, all the tests and helped me a lot in understanding what is going on under the hood.
Basic WinForms, so, should be portable to Linux and Mac as well. Want Eto or Xamarin.Forms instead?
There was a problem hiding this comment.
P.S.: Would you prefer ReadOnlyCollection or something similar instead? Like Keys and Values in Dictionary. The draw-back is that you have to keep it around in a field (for the sake of speed, or make it struct which I don't like much), but that is acceptable.
There was a problem hiding this comment.
I have heard that one of the major speedups they did in one of the KSP versions was that they went through and replaced many of their foreach iterator-based loops with traditional old-school index-counting for loops because Mono's iterator implementation was much slower than just getting items by index (A problem specific to Mono's implementation that Unity uses to be cross-platform, not Microsoft's .NET). That's part of the reason I never had much incentive to want to make an iterator-based wrapper around the stack access.
There was a problem hiding this comment.
Nothing against arrays and indexes, but that would require merging the stack into the CPU, so that derived classes (TesterCPU) can access it. Should not be exposed publicly, that is why I added this read-only interface for it.
There was a problem hiding this comment.
Foreach was fixed in Unity 5.5:
https://unity3d.com/learn/tutorials/topics/performance-optimization/optimizing-garbage-collection-unity-games
There was a problem hiding this comment.
To be honest, I never really understood why the heck in Marianoapp's original implementation, the stack has private protections against the CPU. All that did is make me have to add a ton of dependency inversion interfaces between the two, when really the CPU class should have full permissions to do whatever it likes to the stack. The only real reason I can see to protect the stack would be to deliberately bottleneck the access through a few key APIs, but those APIs really should allow all access and all major operations.
|
Not tested with RemoteTech, I never used that mod, I don't even know how it works. |
|
Separated as requested, sorry for the mess, had to use push force. |
# Conflicts: # src/kOS.Safe/Execution/CPU.cs
|
@Dunbaratu Can you please check that I have resolved the conflicts in CPU.cs correctly? Thx |
|
@firda-cze I'm having a hard time trying to make |
|
@Dunbaratu I have used KDiff3 and used your version whenever it could not resolve it automatically. So, I hope it is fine (most or all conflicts were on the top of the file - removal of the enum, introduction of interrupt priorities, joined list of yielding tasks, some reordering and removal of variables). P.S.: GitHub reported conflicts, that it can no longer merge it automatically, that is why I did it manually (by KDiff3 from SourceTree). Wouldn't touch it otherwise. |
|
The problem I had was just that I can't get git to actually generate the conflicts in the first place. When I fetch your branch, checkout the commit just before your merge of public/develop, then try to merge develop into that myself, git claims there's no conflicts. So I can't see what you saw. I'm not sure why. Hypothetically, that should have worked. |
ghost
mentioned this pull request
|
What is the problem with merging this? Should I recreate boot from archive under current code to get this merged? @Dunbaratu I know you do not like booting from archive, but kOS allows it. So, either dissallow it or make it working. What is the problem? Should I make it as small as possible? I really cannot think of any other reason: You either do not like it (then drop the option alltogether and make it harder for beginners and force them to create boot script to load important stuff to the core before they even start exploring what kOS can do) or you fear it would break something, which is at least understandable for me. I am not perfect, I am not AI but a human and I did make mistakes in my HUGE commit |
|
The lack of an answer to that problem I brought up in July 31 and again on August 1 is why I stopped looking at this PR there. I pointed out that I can't review your merge conflict decisions (like you explicitly asked me to) if I can't get git to actually generate a report of what the conflicts even are. It kept claiming no conflict when I tried to rewind my repo to before your work, and then merge your commits. So you're asking for a review of something I can't see. You fell silent after that, and I was happy for the chance to not have to talk to you for a while. Besides, some of what this PR had was melted together with another PR of yours that hvacengi was still reviewing at the time, and I figured it would get less messy after that got merged. Then there was also the fact that I thought @hvacengi had said he was going to look at this, so I thought my only involvement in this PR was that one line you tagged me on where you asked for the review of the one thing I couldn't see - your merge conflict decisions. Other than that ONE parenthetical side comment, I saw this as being @hvacengi 's merge with me just making side comments but not being the merger. He was usually much more involved in the booting system than I was, and the original issue was raised by him so he knew better whether this PR addressed it well or not. |
| shared.Screen?.Print(string.Format( | ||
| " \n" + | ||
| "Could not boot from {0}\n" + | ||
| "The file is missing\n" + |
There was a problem hiding this comment.
Missing period in the sentence?
There was a problem hiding this comment.
The statement "The file is missing" is a separate sentence that isn't continued on the next line. (Instead a new sentence starts on the next line). So, yeah, it feels like a period is needed before the \n.
There was a problem hiding this comment.
Surprised you don't want it after {0} as that is clearly also separate sentence that isn't continued on the next line. Does not compute
There was a problem hiding this comment.
It's both. For some reason when github showed these comments in the PR thread, it was under line 165 when it shows it under line 166 on the longer diff output, so I didn't see the second sentence on line 166 when replying.
There was a problem hiding this comment.
Does those dot-fixes make it any closer to merging? I don't think so. I will consider creating that minimalistic PR tomorrow... and maybe never. I have my working version and it does not feel like I am of any help here.
| { | ||
| shared.Screen?.Print(string.Format( | ||
| " \n" + | ||
| "Waiting for connection to boot from {0}\n" + |
|
The PR melts together some IStack refactors that don't appear to be in any way related to the issue in with the bug fix. You said this was related to an external tool you use for debugging, but that's not really part of this issue. |
|
Quite sad that you were happy for the chance to not have to talk to me for a while. I have always considered you a team-mate, just like my colleague Ondřej. We disagree occasionally, but always find a way out of it, find a consensus despite our differences, despite our original disagreement and maybe some block and repetition of arguments, but what I value about him is that he does not get stuck. He will repeat his invalid argument but will also, after some repetitions, understand why it is invalid. My other colleague Karel gets stuck indefinitely, repeating nonsense, unable to hear any logic. I can understand that too (fragile psychic, stuck in his self-doubts he is trying to fight the wrong way), but I do not know how to handle him. You seem to me to be very close to Ondřej, but you do not like something about me and resist, a lot, not indefinitely like Karel but still more than Ondřej. I would really like to understand what is the problem. My problem with you all (kOS squad) is the response time - you have your own lives, I can understand that, but if you do not respond in reasonable time (and half a year is really not reasonable time), then there is nothing I can do. About the merge conflict: I thought it is solved by you not finding the conflict (meaning there is no conflict, nothing is needed to be done). Yes, @hvacengi should probably look into this and you @Dunbaratu are here just because I asked you to take a look. The question is still the same: Would it help if I make it minimal? There trully are things that are not needed to fix the problem and I can understand that, that is why I asked - making it minimal should help you (or whomever will review it) do it quicker and merge it. I would like you/anybody merge it as it is, because it would help in testing, which is what I prefer - testing, automated testing, I know I do mistakes, I therefore like to automate tests (and like to write documentation). But that is another story, so be it, I will create another PR with minimal changes to solve the booting from archive problem. |
Do you genuinely mean that? You're asking to open up the floodgates here.
Two separate PR's would work better. One that's a refactor so your debugger can work, and the other that's addressing the bug issue. Putting them together makes it harder for the reviewer who then has to make a judgment call on each line to figure out if it's part of the fix or not. |
Yes I do. |
ghost
closed this
|
@firda-cze I am only saying this because you at least do deserve an explanation. In the text you deleted, you had the rule not to make any judgements about you. Well that makes it impossible to answer the question honestly. Honesty in this case requires pointing out your toxic communication style where you are always on the attack, not treating other team members like equals. (Your deleted text claims you view people as colleagues, but your behavior proves otherwise.) But, I doubt you'll listen. It suffices to say that I'm not alone in this decision. I asked other team members about it, sought out the advice of others who had also watched this exchange so they weren't just relying on my word over yours and gave it a long think, that's why I was away over the weekend. This decision was important, and needed to be made with some distance and time. Communicating with you is painful, making working on kOS a chore instead of a joy. The advantage of another pair of hands on the code is offset by the time lost to having to step away for days to calm down after being browbeaten by your narcissistic refusal to meet on common ground yet again one more time. You annoy more people than just me. I've just had less tolerance of it than others do. There's a reason you were never invited to join the dev team's Slack channel, even though other people who've contributed less have been. Be aware that if you try to use the github comments to complain about this, or blame others yet again, then a ban will be necessary. I don't want to do it, but this is a place for teamwork, not browbeating, so just don't. This toxic commentary has already been waay to public as-is. Just walk away. |
Already fixes #2121 (or I hope so) as much as is currently possible, before we select the proper way of handling the booting from archive. Contains all previous PR's yet to be accepted and the crutial changes I needed for the tester, namely:
CPU.cs:
private->protectedvirtualNoDelegate, that code did nothing,changed to
trigger.EntryPoint < 0.NoDelegateabove).Disposeto the form suggested by MS.Stack.cs, IStack.cs and PseudoNull.cs:
Now for the booting:
run ...command in injected boot the boot script to include something likewait until homeConnection:isConnected., which looks easy, but that would require changing/removingProcessor.CheckCanBoot()and probably replacing it with something like give me the boot the boot script instead, if you want to preserve the interface (I will address that later).YieldFinishedCompile, e.g.YieldCanBoot(or combination of both), that will be waiting until we can really boot. I don't think I am able to create such a thing with my current knowledge of the kOS.Notes:
I would personally remove
IStackbecase there is only one class implementing it and I see no need for it, no vision for any other class implementing it.I would also remove
ICpu, because there are only two implementations - the real one and the one in tests, but the later can be created the same way as myTesterCPUby subclassing theCPUin this PR. It would probably be even better because it is much closer to the original.