AMX_EXEC_CONT (and context switching) support about samp-plugin-jit HOT 38 CLOSED

I disagree. AMX_EXEC_CONT is defined and described as part of the AMX specification, and the same holds for how AMX_ERR_SLEEP should expose the registers when used. Its use may have been uncommon, but it is still part of any complete AMX implementation. Please don't obstruct useful features just because you think you know better.

Since the AMX instructions that must be supported already give a good overview of what can be implemented from this proposal, I think the behaviour of LCTRL and SCTRL can be mirrored for use in this mechanism. If AMX_ERR_SLEEP is raised in a native, all registers simply need to be stored in struct AMX as if LCTRL was used (at the moment, not even pri is stored), and when AMX_EXEC_CONT is used, they just need to be loaded back like for SCTRL. It seems a memcpy between the native stack and the AMX stack is all that remains to be implemented then.

from samp-plugin-jit.

That is completely non-standard operation and really doesn't belong in this plugin. If you want your plugin to do strange custom things with the runtime, fine, but please don't inflict that on other plugins as well.

from samp-plugin-jit.

I wasn't talking about AMX_EXEC_CONT, but context-switching. Please don't force your plugin on everyone else just because you think you know better. I do a lot of funky unusual stuff as well, but it has never been forced on anyone; and never required external compiler/plugin/runtime modifications to use, that would exist even for people not using it.

Also, changing an O(1) operation for an O(N) operation in a plugin designed for speed for a (subjectively) useful feature is objectively bad.

from samp-plugin-jit.

I don't force anything on anyone. There are already context-switching includes for Pawn, using roughly similar mechanisms to mine, and they can be expected to work like a plugin can be expected to work if they operate in a similar fashion. I can do context switching purely in Pawn using #emit and all documented features, so why not request those features for JIT?

Also I don't go around telling everyone to adapt to PawnPlus mechanisms, and indeed, I don't expect this plugin to be fully compatible with mine. However, I was asked to make this issue and so I did, and I would like you to stop responding with "this is bad" when you haven't even heard what it fully encompasses and haven't provided a meaningful reason. Also I'd like to hear from @Zeex about the viability of this, and what would be actually needed to implement it, since my estimates were only based on quick code inspection.

Also to address your point, the memory only needs to be copied when AMX_ERR_SLEEP is raised from a native, and to my knowledge, this doesn't happen in "normal" code. Then and only then can the user expect the members of struct AMX to point to meaningful values. And who knows, maybe the native stack pointer is already located in the AMX memory block; then no copy would be necessary.

from samp-plugin-jit.

I did some profilings to try and determine the costs of your context switching ideas. This is far from perfect, but is comparing two methods of jumping around code fairly arbitrarily - y_inline (using only existing code) vs. PawnPlus tasks, which already do copies of AMX memory:

  ||========================||
  || STARTING PROFILINGS... ||
  ||========================||

Timing "InlineCall[1000]"...
          Mean = 0.00us
          Mode = 0.00us
        Median = 0.00us
         Range = 1.00us

Timing "InlineInit[1000]"...
          Mean = 5.00us
          Mode = 5.00us
        Median = 5.00us
         Range = 1.00us

Timing "PPCall[1000]"...
          Mean = 22689.00us
          Mode = 21555.00us
        Median = 22615.00us
         Range = 2007.00us

Timing "PPInit[1000]"...
          Mean = 1.00us
          Mode = 2.00us
        Median = 2.00us
         Range = 1.00us

*** Profilings: 4

  ||======================||
  || PROFILINGS COMPLETE! ||
  ||======================||

*** Time: 226983ms

A few things:

1. I had to specify repetition counts, because the PP code with defaults just took WAAAY too long, I gave up waiting.
2. Further testing showed the y_inline call time as about half a microsecond, the resolution was just insufficient with only 1000 repetitions.
3. The init code was limited in repetitions for entirely different reasons (a bug in SetTimerEx).

That's a slowdown of around FORTY-THOUSAND times! That's far far worse than I thought even when I thought it was a bad idea before.

Code:

#pragma option -d0
#pragma option -O1

#define RUN_PROFILINGS

#include <a_samp>
#include <PawnPlus>
#include <YSI_Core\y_profiling>
#include <YSI_Coding\y_inline>

new Handle:gHandle;

forward ThePP();

public ThePP()
{
	new Task:task = task_new();
	gHandle = handle_new(task, .weak = false);
	handle_acquire(gHandle);
	task_keep(task, true);
	for ( ; ; )
	{
		task_await(task);
		task_reset(task);
	}
}

ProfileInit:PPCall()
{
	CallLocalFunction("ThePP", "");
}

Profile:PPCall[1000]()
{
	new Task:task = Task:handle_get(gHandle);
	task_set_result(task, true);
}

ProfileClose:PPCall()
{
	new Task:task = Task:handle_get(gHandle);
	handle_release(gHandle);
	task_delete(task);
}

Profile:PPInit[1000]()
{
	new Task:task = task_new();
	new Handle:handle = handle_new(task, .weak = false);
	handle_acquire(handle);
	handle_release(handle);
	task_delete(task);
}

DoInit(Func:x<>)
{
	Indirect_Claim(x);
	Indirect_Release(x);
}

// This needs to be restricted due to a bug in `SetTimer` when calling it
// thousands of times in a row.
Profile:InlineInit[1000]()
{
	inline const TheInline()
	{
	}
	DoInit(using inline TheInline);
}

new Func:gFunc<>;

DoSave(Func:x<>)
{
	gFunc = x;
	Indirect_Claim(x);
}

ProfileInit:InlineCall()
{
	inline const TheInline()
	{
	}
	DoSave(using inline TheInline);
}

Profile:InlineCall[1000]()
{
	@.gFunc();
}

ProfileClose:InlineCall()
{
	Indirect_Release(gFunc);
}

main()
{
}

I'd make a fiddle for it, but you can't specify [email protected] on that site yet.

from samp-plugin-jit.

I don't force anything on anyone. There are already context-switching includes for Pawn, using roughly similar mechanisms to mine, and they can be expected to work like a plugin can be expected to work if they operate in a similar fashion.

Includes are not forced, you want them, you include them; you don't want them, you don't include them. You're trying to put features in entirely unrelated systems (the VM no less) to support your single unique use-case, regardless of whether people use your stuff or not. How is that not forcing it on them?

I can do context switching purely in Pawn using #emit and all documented features, so why not request those features for JIT?

You literally answered your own question in the same sentence. There are other reasons, but "I can do [it]" already is a very good one.

you haven't even heard what it fully encompasses

Enlighten me.

haven't provided a meaningful reason.

1. You want to standardise this behaviour (your words). This means it becomes part of all pawn runtimes, which means it should have wide general applications, but you're trying do to an end-run and go straight to Zeex to get it pushed in.

2. Since this is the VM, and would be standardised, it should fit in with PAWNs general rules and ethos (something you've repeatedly shown extreme disdain for). One of which is static memory usage. Plugins and functions do have dynamic memory, but the runtime doesn't (at least not during runtime). This would completely remove that guarantee.

3. It's horribly slow (see above).

4. You yourself said it isn't needed because you can already do the same thing. In a plugin, where this non-standard behaviour should go.

Also I'd like to hear from @Zeex about the viability of this, and what would be actually needed to implement it, since my estimates were only based on quick code inspection.

No-one is debating the feasibility of implementation.

Also:

• You outright reject community standards, yet apparently I'm the one that thinks they know better than everyone else.

• You want to make this a standard, but also claim you aren't forcing it on people, it can't be both. You clearly want to push your way of doing things on everyone else, yet apparently I'm the one that thinks they know better than everyone else.

• I've been warning you for over a year now that what you were trying to do wouldn't work with the JIT. What's your response? Ignore me, then declare that clearly the JIT is wrong and should change, not the other way around. Yet apparently I'm the one who thinks they know better than everyone else.

• When developing YSI, I've always maintained that it should flex to fit in PAWN, not the other way around. When the compiler updates, I update it. I've even proposed compiler changes that would make YSI code more complex because of the way it exploits certain strange behaviours. But I'm of the opinion that if your code manipulates the VM at a low-level (emit, plugins, etc) then all guarantees are out. Things can change under your feet and you should adapt, because at that level you should be aware of the changes. You seem to be of the opinion that the VM and low-level code should change to suit your specific needs. Yet apparently I'm the one that thinks they know better than everyone else.

In short

If you want your own custom VM behaviour, keep it in your own custom VM.

from samp-plugin-jit.

I feel like I am repeating myself, but whatever.

Includes are not forced, you want them, you include them; you don't want them, you don't include them.

It was not me who made JIT incompatible with PawnPlus on purpose. Both systems do uncommon things with the AMX, but I try to stay within limits of the AMX API and not modify the core implementations. Indeed I can make a fork of JIT and add the support for those features, but it would take me longer time, and the end result might not still be compatible fully. Then I could try to make a pull request, but I can imagine you will be there to prevent it from being merged.

You're trying to put features in entirely unrelated systems (the VM no less) to support your single unique use-case, regardless of whether people use your stuff or not. How is that not forcing it on them?

I am trying to make the AMX implementation the JIT provides to be complete. You have never addressed the fact that AMX_EXEC_CONT is not supported by JIT at all. If I try to start a debate about finishing a documented but unimplemented feature of the AMX runtime in a plugin which implements the AMX runtime, how is that beneficial only to my single use-case? I don't request something to be at a specific fixed memory address, I don't request the order of specific operations to be fixed; I just request the implementation to stick to the documentation when possible.

You literally answered your own question in the same sentence. There are other reasons, but "I can do [it]" already is a very good one.

A single feature can be done either from Pawn, or using the AMX API. The Pawn side support is implemented here, the AMX API side support is not. If you believe the features that can be fully implemented in Pawn only are the only features that every SA-MP programmer ever needs, you're beginning to think like Kalcor.

Enlighten me.

It's up to the debate with Zeex what would actually be needed to implement. I have already stated what I think needs to be done, in every post I made.

You want to standardise this behaviour (your words). This means it becomes part of all pawn runtimes, which means it should have wide general applications, but you're trying do to an end-run and go straight to Zeex to get it pushed in.

This behaviour is the standard. When I stated that PawnPlus stores the heap and the stack in a separate memory and then loads it back, and asked what YSI does, you told me it does the same (apart from the heap). Do you know why? Because YSI has access to LCTRL and SCTRL, because these are AMX opcodes (and they are explicitly implemented by JIT). If I can have access to those registers in Pawn (and they seem to work), why cannot I have access to those registers in C++? Oh yes, because AMX_ERR_SLEEP (used by Pawn sleep statement) is not supported. We are back to the beginning.

Since this is the VM, and would be standardised, it should fit in with PAWNs general rules and ethos (something you've repeatedly shown extreme disdain for). One of which is static memory usage. Plugins and functions do have dynamic memory, but the runtime doesn't (at least not during runtime). This would completely remove that guarantee.

Not sure where you're going with this one − I have no need for this plugin to do dynamic memory allocation. And both of us do context switching within the terms of what the runtime can offer.

It's horribly slow (see above).

It might be, but I hope we both agree that some features are best used in certain cases. PawnPlus might be used to accomplish more than what YSI can ever do, but when YSI can do compile-time safety, it has to do runtime safety and stick to its principles (when switching contexts). There's where the increase comes from. Anyway, the speed of PawnPlus is not relevant (not all code in a script runs on PawnPlus).

You yourself said it isn't needed because you can already do the same thing. In a plugin, where this non-standard behaviour should go.

I said I can do context switching in pure Pawn (with #emit). If I want to support dynamic allocation in combination with context switching that relies on the same principles (and make it more powerful in the process), I have to go with using a plugin, but I cannot use the same principles in the plugin, because the API here is not fully supported. Again, I can make a fork of JIT (because I cannot make it work in PawnPlus alone due to the way it works), but I thought you above everyone else could see the impracticability of having two versions of JIT that could be made into one.

No-one is debating the feasibility of implementation.

That's the issue.

You outright reject community standards, yet apparently I'm the one that thinks they know better than everyone else.

If these are truly the community standards, then it's up to the community (and not you) to want me to keep the standards upheld. So far, only you object most of the time. I follow the standards of the Pawn runtime and not of a community which tells me to implement their standards or face utter damnation.

You want to make this a standard, but also claim you aren't forcing it on people, it can't be both. You clearly want to push your way of doing things on everyone else, yet apparently I'm the one that thinks they know better than everyone else.

Wrong and wrong. We both utilize the same "standard", but I don't have access to it from the AMX API side, and you have access to it from the Pawn side. And again, I was told to make this issue by someone who wanted to use PawnPlus together with JIT, and I realized PawnPlus is not the plugin that needs to be modified.

I've been warning you for over a year now that what you were trying to do wouldn't work with the JIT. What's your response? Ignore me, then declare that clearly the JIT is wrong and should change, not the other way around. Yet apparently I'm the one who thinks they know better than everyone else.

And yet despite your holy crusade on PawnPlus and my ideas, even resorting to plain lies in one case, I have prevailed. You were not ignored; I listened to you when you called for additional type safety, I added generics (yet not many people use them), and indeed, I do understand where the limitations of my techniques are (threading is the prime example – can be used in some cases, not in others). But in this case, it seems to me that when you already do the same specific thing in the same way (y_inline), your only goal is to make sure PawnPlus doesn't benefit from the same thing, because it doesn't stick to the "community" ideals, ideals whose formation I wasn't even a part of.

But I'm of the opinion that if your code manipulates the VM at a low-level (emit, plugins, etc) then all guarantees are out. Things can change under your feet and you should adapt, because at that level you should be aware of the changes. You seem to be of the opinion that the VM and low-level code should change to suit your specific needs. Yet apparently I'm the one that thinks they know better than everyone else.

And indeed, if a change happens that makes some people's lives harder, yet is a step in the right direction (like const-correctness), I can see that. However, here a change is debated that doesn't go against the principles of Pawn or AMX, doesn't break or impair existing code in any way, and helps a wider range of people than just me. Why? Because it is specified in the Pawn Implementer Guide goddamnit!

In short

I don't think you listen to me. The support of AMX_ERR_SLEEP/AMX_EXEC_CONT (and the access to "LCTRL"/"SCTRL" from that place) is all I need, or what I seem to need now that I know better what needs to be done. That's not PawnPlus-specific.

from samp-plugin-jit.

Yes, this can be done, should be easy I think. But since it adds a check after all native calls it make slow things down a little bit... maybe it should be off by default and on if some special config variable is set.

from samp-plugin-jit.

@Zeex No problem, if you think so, but the check is already there - asm_.cmp(edi, AMX_ERR_NONE);. Only halt_helper_label_ needs to be modified in case of a native call, and that will only be called in case of an error, thus not many times.

from samp-plugin-jit.

@IllidanS4 Do you have a test script that checks all the registers, etc by any chance? I probably can write my own but I'm afraid it will take forever (my AMX assembly is a little rusty).

from samp-plugin-jit.

@Zeex No, I usually debug in Visual Studio and observe the registers there.
What do you mean by checking them? To see if they are saved, you can try any asynchronous PawnPlus function:

public OnFilterScriptInit()
{
    new var = 1;
    printf("A %d", var);
    wait_ms(1000);
    printf("B %d", var);
}

from samp-plugin-jit.

I added some support for sleep / AMX_EXEC_CONT in the sleep branch. It probably has some bugs (I'm almost sure of it).

@IllidanS4 Please see if it works with PawnPlus.

from samp-plugin-jit.

@Zeex Thanks for the update! I have tested the branch, and the basic thing seems to be working, but there are still a couple of issues I found:

• A value returned from a native function which raises AMX_ERR_SLEEP isn't saved in pri. The same thing could be emulated in Pawn via sleep val;, which should put the value in pri and in *retval. At the moment, only garbage is there for both methods.
• reset_hea and reset_stk are set to 0 when amx_Exec returns. These "registers" are necessary for resuming the machine in the correct state, since they contain the backup values that are applied when an error occurs in the code. They should contain the values of hea and stk that were there before the call.

I was able to pause and restore the state of the machine (when I provided my own reset_hea and reset_stk), but PawnPlus uses the return value to determine which operation is to be performed.

from samp-plugin-jit.

@IllidanS4 I pushed a couple of fixes that should address those issues, please test them with PawnPlus.

from samp-plugin-jit.

@Zeex Testing right now, but reset_stk seems a bit off according to what it normally is. It is 4 bytes below the original value, so now when amx_Exec was called and stk was 4194620, after the call reset_stk is 4194616.

It is not critical to have these values for PawnPlus (I can just store stk and hea prior to the call to amx_Exec); I just thought they are necessary for correct execution of AMX. How were you restoring the stack and heap to their initial values in case of an error prior to this? If they weren't unrolled, that would mean old data are left in the memory and in case of many AMX errors, memory leak would occur.

from samp-plugin-jit.

Yes, it was off by 4 bytes before the last commit, I think it's in sync right now. STK was also off by 2 cells. Are you sure you're testing with the latest commit?

from samp-plugin-jit.

Yes, I redownloaded the project just to be sure and PawnPlus reported the mismatch in two different filterscripts:

[PawnPlus] STK will be restored to 0x4024 (originally 0x4028).
[PawnPlus] STK will be restored to 0x400138 (originally 0x40013C).

Meaning before calling amx_Exec, stk was 0x4028, but now reset_stk is 0x4024.

It also seems to crash in some cases if the different value is used to restore the AMX, but works if the original one is used instead.

from samp-plugin-jit.

Hmm, that's strange. Can you show some codes to reproduce that bug? preferably short

I probably will be able to look into it next weekend

from samp-plugin-jit.

Never mind, I see what you mean.

from samp-plugin-jit.

The latest commit seems to be working so far, thanks! Will let you know in case of further issues.

from samp-plugin-jit.

Strange, now I am testing this again, I get the same mismatches:

[PawnPlus] STK will be restored to 0x400174 (originally 0x40017C).

It doesn't crash, but I get breakpoints hit in Visual Studio when I debug the server...

from samp-plugin-jit.

Breakpoints were added on purpose, I'll put them behind a flag later. You can comment them out in EmitDebugBreakpoint.

Regarding the stack issue, do you have an example code to reproduce it?

Update: added jit_debug option

from samp-plugin-jit.

BTW you can add your own breakpoints using #emit break (in that case you probably want to compile with -d0).

from samp-plugin-jit.

I wasn't complaining about the breakpoints, just surprised. Going to check the stk difference now.

from samp-plugin-jit.

OK 😄

It would be great if there was a way to generate debug info at runtime on Windows and set function breakpoints via the VS debugger, but it seems to be not possible (though GDB has some support for this). So the best debug method I could think of was to add a bunch of int3s.

from samp-plugin-jit.

When I run the testing script, I get this output:

[sleep] Executing sleep_callback
[sleep] before exec: frm = 0
[sleep] before exec: stk = 4064
[sleep] before exec: hea = 68
[sleep] Scheduled continuation at +1000 ms
[PawnPlus] STK will be restored to 0x4060 (originally 0x4064).
[sleep] after exec: frm = 0
[sleep] after exec: stk = 4060
[sleep] after exec: hea = 68
[sleep] Continuing execution
[sleep] cip = a4
[sleep] pri = c0ffee
[sleep] alt = 0
[sleep] frm = 0
[sleep] stk = 4060
[sleep] hea = 68
[sleep] reset_stk = 4060
[sleep] reset_hea = 68
Hello! x = 48
Right?

The mismatch is there as well, since stk started at 4064, but went down to 4060. I tried to add exit; at the end of sleep_callback to see if the stack could be correctly restored to 4064, but it crashed.

Perhaps something to do with paramcount or the cell that contains it?

Amusingly enough, PawnPlus does warn about the mismatch even though it doesn't actually restore anything since it doesn't handle unrecognized sleeps (I also tried to disable it but the behaviour stays the same).

from samp-plugin-jit.

Fixed

from samp-plugin-jit.

There seems to be an issue unrelated to sleep in this branch. I can't describe it better, but it seems some callbacks aren't executed at all, even though there is no sleep in them. I'll see which commit caused that.

from samp-plugin-jit.

@Zeex 490d201 seems to be the commit that introduced the issue. I tried testing out my gamemode and I couldn't even spawn.

Also is adding jit_sleep 1 to server.cfg sufficient to enable the feature? It doesn't seem to do anything when I tried with the latest commit.

from samp-plugin-jit.

Yes, jit_sleep 1 enables sleep support.

Also: fixed the crash issue.

from samp-plugin-jit.

There was a bug in callback return value code, should be OK now (BTW the fix is in master, not the sleep branch).

from samp-plugin-jit.

Callbacks now seem to return correctly, but I still get some crashes...

Edit: amx_FindPublic is called with funcname==nullptr causing PawnPlus to crash. I shall fix the crash on my side but I wonder why it happens.

Edit 2: amx_FindPublic in SA-MP crashes anyway on nullptr. Something is calling CallLocalFunction with an empty string.

Edit 3: print("A"); at one place in my code prints (null). This happens after a call to CallLocalFunction (which succeeds). Perhaps the data pointer is incorrect?

from samp-plugin-jit.

Have you tried the latest one? I fixed a couple of issues there.

from samp-plugin-jit.

Still the same, unfortunately. There are also some heap underflows apparently. It crashes even with jit_sleep 0.

from samp-plugin-jit.

works fine for me

can you show some examples?

from samp-plugin-jit.

Try this: https://ci.appveyor.com/api/buildjobs/22db0ednohiqxtv9/artifacts/jit-2.3.1-45-gd5c2410-win32.zip

from samp-plugin-jit.

Nope, the same issue with this one as well. I will try to create a reproducible example, but I have no idea what operation causes it.

from samp-plugin-jit.

I've released a new version (2.4) with sleep support included behind a config flag (jit_sleep), so I'm closing this issue.. If you are still experiencing problems, please open a new one.

from samp-plugin-jit.