Conversation
…lp improve performance. Also made numerous changes to section of code that previous used the global_lock to work correctly with the new CriticalSection
spin locks in ChezScheme for critical sections. Optimizations for loading DLLs.
Replaces Win32 Semaphore objects with Win32 CriticalSection objects. This is lightweight and also allows for future use of Win32 ConditionVariable objects. In addition to this, Win32 CriticalSection objects support a spinlock count which can help performance in sections that can be very short in terms of execution times. Replaces LoadLibrary calls with GetModuleHandle when appropriate. In the code base, there were some calls that loaded libraries that have to be present in any executable (e.g. kernel32.dll). The correct thing is to use GetModuleHandle to avoid incrementing the reference count for a DLL. Also, in cases in which all the API calls are available since Windows Server 2008, the code just sets function pointers directly versus using GetProcAddress. There is also an update to how the console is created and deleted as needed. The older version used API calls that are no longer needed. Also, the console uses flags to make sure the console supports more virtual terminal functions (working more like a Unix terminal)
|
Not sure why the tests are failing here when they run on my local machine. I don't want to give the impression that I am making pull requests without doing the basic tests. Well, it seems that both my local environment and the build environment are giving the same error. This is hopeful. |
|
This all seems to stem from how |
…errno." This reverts commit 1ae81c1.
|
So, this seems to be an error that is thrown from the |
|
It seems that touching anything in |
…the build target. Updated ChezScheme\mats\foreign.ms to do the same.
| rktio_global_lock = CreateSemaphore(NULL, 1, 1, NULL); | ||
| BOOL was_created = InterlockedCompareExchangeAcquire(&rktio_global_cs_created, TRUE, FALSE); | ||
| if (!was_created) | ||
| InitializeCriticalSectionEx(&rktio_global_cs, CS_SPINCOUNT, CRITICAL_SECTION_NO_DEBUG_INFO); |
There was a problem hiding this comment.
This doesn't seem right. If rktio_init could be called in multiple threads, then was_created could be set to true before rktio_global_cs is initialized. But a constraint on rktio_init is that the first call must be non-concurrent with any other call. So, just use gloabl_cs_created directly (made static and without the rktio_ prefix, since it shouldn't be sued outside this .c file)?
There was a problem hiding this comment.
This new version is ok, although using InterlockedCompareExchangeAcquire here still seems more than is needed. The global_cs_created could just guard the call to InitializeCriticalSectionEx and then get set afterward.
| int errno_val; | ||
|
|
||
| #ifdef WIN32 | ||
| { |
There was a problem hiding this comment.
This looks like probably a good idea, but Chez Scheme changes will need to go through https://github.com/cisco/ChezScheme, and then I keep merged changes in sync. (In very rare cases, we merge Chez Scheme changes here first to try them out, but this doesn't seem like a case where that is needed.)
| if (iconv_is_ready) | ||
| return; | ||
|
|
||
| EnterCriticalSection(&rktio_global_cs); |
There was a problem hiding this comment.
Doesn't the iconv_is_ready check need to be within the critical section?
versus dynamically getting their address from a loaded DLL. This is possible because all the needed API calls that are not supported are versions of Windows that are so out of date it is not reasonable to support them. These changes require the use of the `Psapi.lib` and build files have been modified to support that. Changes to ChezScheme were reverted.
mflatt
left a comment
mflatt
left a comment
There was a problem hiding this comment.
Thanks for the revision! Looks like things are moving in a good direction.
| set MORE_CFLAGS= | ||
| set MORE_LDFLAGS= | ||
| set MORE_CFLAGS=-DPSAPI_VERSION=2 | ||
| set MORE_LDFLAGS=-DPSAPI_VERSION=2 |
There was a problem hiding this comment.
These should probably be in "build.zuo", instead. The intent is that anything always needed goes there, while winfig.bat has things that make sense to change.
| RKTIO_LIB = ..\..\build\librktio.lib | ||
| BASE_WIN32_LIBS = WS2_32.lib Shell32.lib User32.lib Winmm.lib | ||
| WIN32_LIBS = $(BASE_WIN32_LIBS) RpCrt4.lib Ole32.lib Advapi32.lib | ||
| WIN32_LIBS = $(BASE_WIN32_LIBS) PsApi.lib RpCrt4.lib Ole32.lib Advapi32.lib |
There was a problem hiding this comment.
I think this Makefile isn't used, and it's a leftover from the old system. So, ok to include this change, but also ok to drop it, and I can remove the file separately either way.
|
Besides #5592, I'll follow up later with a Chez Scheme patch derived from this one. |
2 participants
Checklist
Description of change
This is a set of changes around replacing Win32 Semaphore with Win32 CriticalSections and related small changes. This is primary performance based as well as avoiding some bad patterns around loading DLLs.