Skip to content

Python and Skia don't mix #32

Open

Description

@peardox

I'm not positive about the exact criteria yet

Basically imports with Skia fully enabled causes Skia to hit problem with floating point

Take anything with an import and add this to your dproj (after installing Skia of course) and everything will go badly wrong.

Masking the FPU exception doesn't seem to help

As I don't know which package is causing the issue this is posted here

uses
  Skia, Skia.FMX, , ....

begin
  GlobalUseSkia := True;
  GlobalUseSkiaRasterWhenAvailable := False;
.
.

Activity

  1. viniciusfbb commented on Aug 19, 2022

    @viniciusfbb

    Hello,
    The way Python4delphi is changing exception masks is not secure. Care is needed mainly to remove exceptions from the mask.
    It's something that affects the entire application and other libraries like Skia, OpenGL, ActiveX, etc...

    The problem is specifically in the MaskFPUExceptions function in PythonEngine.pas, which is used in the initialization of P4D-Data- Sciences

    There are a few ways to resolve this. One of them would be to create a class to store the mask's state, change it, so that after a try finally you can restore the previous mask.

  2. peardox commented on Aug 19, 2022

    @peardox
    Author

    @viniciusfbb Thanks for the rapid reply and explanation. I sorta thought this was most likely MaskFPUExceptions (hence the issue being tentatively in this repo)

    I wrote a replacement for MaskFPUExceptions that stores the current GetExceptionMask when called with True in a global then replaces it when called with false. This worked a treat and Skia now works perfectly again (Skia was really acting up without this fix...).

    @lmbelo While my hack works I'm not submitting a PR for this one as this is a complicated issue which is situational - my 'fix' works for my app but other packages may well mess with the Exception Mask so a more robust solution is really required.

    For reference here's what I did - you can enable/disable it via the define (good for testing)...

    {$DEFINE USESAFEMASK}
    
    var
      FPUMASK: TArithmeticExceptionMask;
    
    procedure SafeMaskFPUExceptions(ExceptionsMasked : boolean;
      MatchPythonPrecision : Boolean = True);
    
    implementation
    
    uses
      Maths;
    
    {$R *.fmx}
    
    procedure SafeMaskFPUExceptions(ExceptionsMasked : boolean;
      MatchPythonPrecision : Boolean);
    begin
      {$IFDEF USESAFEMASK}
      {$IF Defined(CPUX86) or Defined(CPUX64)}
      if ExceptionsMasked then
        begin
        FPUMASK := GetExceptionMask;
        SetExceptionMask([exInvalidOp, exDenormalized, exZeroDivide,
          exOverflow, exUnderflow, exPrecision]);
        end
      else
        SetExceptionMask(FPUMASK);
      {$WARN SYMBOL_PLATFORM OFF}
      {$IF Defined(FPC) or Defined(MSWINDOWS)}
      if MatchPythonPrecision then
          SetPrecisionMode(pmDouble)
        else
          SetPrecisionMode(pmExtended);
      {$WARN SYMBOL_PLATFORM ON}
      {$IFEND}
      {$IFEND}
      {$ELSE}
        MaskFPUExceptions(ExceptionsMasked, MatchPythonPrecision);
      {$IFEND}
    end;
    
    
  3. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    The exception mask is undone after the importation of any of the Python packages. If you have a look at the exception mask routine you'll see that the exception mask is being undone (for CPUX86 and CPUX64) to Delphi's default.
    If we're changing exception mask in somewhere out of modules importation, we should be restoring to the defaults when we're done.
    Can you show specifically where it is breaking down? Which platform are you targeting to?
    Hope I can be of any help.

  4. self-assigned this
    on Aug 21, 2022
  5. peardox commented on Aug 21, 2022

    @peardox
    Author

    Ahh, that explains why using afterimport to reset the mask didn't work...

    I tried two methods...

    import(packagea)
    import(packageb)
    import(packagec)

    with each of those using shared beforeinstall, afterinstall, beforeimport, afterimport which calls my SafeMaskFPUExceptions() function - this didn't work :(

    So, I've not got

    SafeMaskFPUExceptions(True)
    import(packagea)
    SafeMaskFPUExceptions(False)

    See https://github.com/peardox/Lartis/blob/main/PythonSystem.pas#L501

  6. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    You should decorate it by a try finally block.

    MaskFPUExceptions(true);
    try
      //do whatever you want to do
    finally
      MaskFPUExceptions(false);
    end;
    
  7. peardox commented on Aug 21, 2022

    @peardox
    Author

    I'm going to - this was a recent test, still unfinished - no proper error handlers yet

    I just discovered that I need to handle errors properly anyway (Linux screws up horribly ATM - Issue coming soon...)

  8. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    You need to get extra attention when targeting installed Python versions on Linux. You might hit across known issues reported before here. Have a look in all closed issues when you're going afters Linux errors.
    Only for the sake of getting you ahead, you can't use apt installed Python working with the DelphiFMX library.
    We need a Python executable linked statically or dynamically with the interpreter shared library, or an executable that exposes interpreter's symbols some how. The is not the case of Python available by apt.

  9. peardox commented on Aug 21, 2022

    @peardox
    Author

    I was using an Embedded Python 3.9 - which is why the issue is listed here

  10. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    Our embeddables should work fine on any of the provided platforms.

  11. viniciusfbb commented on Aug 21, 2022

    @viniciusfbb

    @lmbelo, you didn't understand my post. The P4D-Data-Sciences code is wrong and python4delphi's MaskFPUExceptions implementation should be changed as well. I will explain it better.

    MaskFPUExceptions code from python4delphi:

    procedure MaskFPUExceptions(ExceptionsMasked : boolean;
    MatchPythonPrecision : Boolean);
    begin
    {$IF Defined(CPUX86) or Defined(CPUX64)}
    if ExceptionsMasked then
    SetExceptionMask([exInvalidOp, exDenormalized, exZeroDivide,
    exOverflow, exUnderflow, exPrecision])
    else
    SetExceptionMask([exDenormalized, exUnderflow, exPrecision]);
    {$WARN SYMBOL_PLATFORM OFF}
    {$IF Defined(FPC) or Defined(MSWINDOWS)}
    if MatchPythonPrecision then
    SetPrecisionMode(pmDouble)
    else
    SetPrecisionMode(pmExtended);
    {$WARN SYMBOL_PLATFORM ON}
    {$IFEND}
    {$IFEND}
    end;

    Note that the code above arbitrarily changes the exception mask to:

    [exInvalidOp, exDenormalized, exZeroDivide, exOverflow, exUnderflow, exPrecision] when the ExceptionsMasked parameter is True
    [exDenormalized, exUnderflow, exPrecision] when the ExceptionsMasked parameter is False

    Now, look at one of the P4D-Data-Sciences import code:

    procedure TTorchVision.ImportModule;
    begin
      MaskFPUExceptions(true);
      try
        inherited;
      finally
        MaskFPUExceptions(false);
      end;
    end;

    As it stands, if the exception mask before the code above is empty [], after import it will be [exDenormalized, exUnderflow, exPrecision]. Likewise, if the exception mask is exAllArithmeticExceptions (all exceptions), which is what happens when using OpenGL, D2D, GDIP, Skia, among others, after import the mask will be [exDenormalized, exUnderflow, exPrecision]. And there is no way to predict the mask that will be configured in the application when it arrives at the P4D-Data-Sciences import code, there are many variants, there are several other libraries that change like the Delphi XML lib itself, and it can even vary according to the application (VCL or FMX).

    So, the correct thing is: save the mask value, change it and after importing the P4D-Data-Sciences, restore the previous mask value and not arbitrarily change it to [exDenormalized, exUnderflow, exPrecision].

    I made a possible fix for both codes. First for python4delphi's MaskFPUExceptions:

    procedure MaskFPUExceptions(ExceptionsMasked: Boolean;
      var StoredMask: TArithmeticExceptionMask; MatchPythonPrecision: Boolean);
    begin
      {$IF Defined(CPUX86) or Defined(CPUX64)}
      if ExceptionsMasked then
        StoredMask := SetExceptionMask(GetExceptionMask + [exInvalidOp, exDenormalized, exZeroDivide,
          exOverflow, exUnderflow, exPrecision])
      else
        SetExceptionMask(StoredMask + [exDenormalized, exUnderflow, exPrecision]); // Maybe a SetExceptionMask(StoredMask); would be better, but I'm not sure if removing the " + [exDenormalized, exUnderflow, exPrecision]" would cause problems in python4delphi
      {$WARN SYMBOL_PLATFORM OFF}
      {$IF Defined(FPC) or Defined(MSWINDOWS)}
      if MatchPythonPrecision then
          SetPrecisionMode(pmDouble)
        else
          SetPrecisionMode(pmExtended);
      {$WARN SYMBOL_PLATFORM ON}
      {$IFEND}
      {$IFEND}
    end;

    And now one of the P4D-Data-Sciences import codes:

    procedure TTorchVision.ImportModule;
    var
      StoredMask: TArithmeticExceptionMask;
    begin
      MaskFPUExceptions(true, StoredMask);
      try
        inherited;
      finally
        MaskFPUExceptions(false, StoredMask);
      end;
    end;
  12. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    @lmbelo, you didn't understand my post. The P4D-Data-Sciences code is wrong and python4delphi's MaskFPUExceptions implementation should be changed as well. I will explain it better.

    MaskFPUExceptions code from python4delphi:

    procedure MaskFPUExceptions(ExceptionsMasked : boolean;
    MatchPythonPrecision : Boolean);
    begin
    {$IF Defined(CPUX86) or Defined(CPUX64)}
    if ExceptionsMasked then
    SetExceptionMask([exInvalidOp, exDenormalized, exZeroDivide,
    exOverflow, exUnderflow, exPrecision])
    else
    SetExceptionMask([exDenormalized, exUnderflow, exPrecision]);
    {$WARN SYMBOL_PLATFORM OFF}
    {$IF Defined(FPC) or Defined(MSWINDOWS)}
    if MatchPythonPrecision then
    SetPrecisionMode(pmDouble)
    else
    SetPrecisionMode(pmExtended);
    {$WARN SYMBOL_PLATFORM ON}
    {$IFEND}
    {$IFEND}
    end;

    Note that the code above arbitrarily changes the exception mask to:

    [exInvalidOp, exDenormalized, exZeroDivide, exOverflow, exUnderflow, exPrecision] when the ExceptionsMasked parameter is True [exDenormalized, exUnderflow, exPrecision] when the ExceptionsMasked parameter is False

    Now, look at one of the P4D-Data-Sciences import code:

    procedure TTorchVision.ImportModule;
    begin
      MaskFPUExceptions(true);
      try
        inherited;
      finally
        MaskFPUExceptions(false);
      end;
    end;

    As it stands, if the exception mask before the code above is empty [], after import it will be [exDenormalized, exUnderflow, exPrecision]. Likewise, if the exception mask is exAllArithmeticExceptions (all exceptions), which is what happens when using OpenGL, D2D, GDIP, Skia, among others, after import the mask will be [exDenormalized, exUnderflow, exPrecision]. And there is no way to predict the mask that will be configured in the application when it arrives at the P4D-Data-Sciences import code, there are many variants, there are several other libraries that change like the Delphi XML lib itself, and it can even vary according to the application (VCL or FMX).

    So, the correct thing is: save the mask value, change it and after importing the P4D-Data-Sciences, restore the previous mask value and not arbitrarily change it to [exDenormalized, exUnderflow, exPrecision].

    I made a possible fix for both codes. First for python4delphi's MaskFPUExceptions:

    procedure MaskFPUExceptions(ExceptionsMasked: Boolean;
      var StoredMask: TArithmeticExceptionMask; MatchPythonPrecision: Boolean);
    begin
      {$IF Defined(CPUX86) or Defined(CPUX64)}
      if ExceptionsMasked then
        StoredMask := SetExceptionMask(GetExceptionMask + [exInvalidOp, exDenormalized, exZeroDivide,
          exOverflow, exUnderflow, exPrecision])
      else
        SetExceptionMask(StoredMask + [exDenormalized, exUnderflow, exPrecision]); // Maybe a SetExceptionMask(StoredMask); would be better, but I'm not sure if removing the " + [exDenormalized, exUnderflow, exPrecision]" would cause problems in python4delphi
      {$WARN SYMBOL_PLATFORM OFF}
      {$IF Defined(FPC) or Defined(MSWINDOWS)}
      if MatchPythonPrecision then
          SetPrecisionMode(pmDouble)
        else
          SetPrecisionMode(pmExtended);
      {$WARN SYMBOL_PLATFORM ON}
      {$IFEND}
      {$IFEND}
    end;

    And now one of the P4D-Data-Sciences import codes:

    procedure TTorchVision.ImportModule;
    var
      StoredMask: TArithmeticExceptionMask;
    begin
      MaskFPUExceptions(true, StoredMask);
      try
        inherited;
      finally
        MaskFPUExceptions(false, StoredMask);
      end;
    end;

    @viniciusfbb does Skia changes the exception mask for the whole app life cycle?

  13. peardox commented on Aug 21, 2022

    @peardox
    Author

    Just to complicate things ARM can emulate the FPU mask and Mac can be x64 or aarch64

  14. viniciusfbb commented on Aug 21, 2022

    @viniciusfbb

    @viniciusfbb does Skia changes the exception mask for the whole app life cycle?

    Yes! OpenGL too.

  15. 4 remaining items

  16. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    Well, regarding Windows x86 (made by FLDCW and FSTCW ???) and x64, the FPSCR flag equivalent is saved across thread context switches. I'm almost sure ARM has a similar approach, I think they save it in the _FPSCR array (I need to confirm this).
    Those masks we are setting up on P4D are the ones required by Python.

  17. viniciusfbb commented on Aug 21, 2022

    @viniciusfbb

    Well, regarding Windows x86 and x64, the FPSCR flag is saved across thread context switches. I'm almost sure ARM has a similar approach, I think they seve it in the _FPSCR array (I need to confirm this). Those masks we are setting up on P4D are the ones required by Python.

    Not quite. Change the main, run a thread and you will see the value of the mask of the main in the thread... Until a few years ago SetExceptionMask was not even thread safe, but I believe this has already been fixed.

    Anyway, just changing the import of P4D-Data-Sciences the way you suggest is not the way I think is the most correct but it will hardly cause problems.

  18. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    Well, just tried it for the sake of testing. Definitely changing it on a second thread doesn't affect the main thread. It proves that it is saved across thread context switches.

    Screen Shot 2022-08-21 at 15 20 59

    Screen Shot 2022-08-21 at 15 21 21

    Still not sure that doing that change is the way to go.

  19. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    As did by FMX.Canvas.GDIP, changes made on masks are done per invocation and undone after it. Changing exception masks for app life cycle can be risk.

    Screen Shot 2022-08-21 at 15 30 25

  20. viniciusfbb commented on Aug 21, 2022

    @viniciusfbb

    I didn't say that changes to background threads would affect the main thread's exception mask. In my view, threads being created in the background would have the initial values of exception mask of the main thread.

    However, I have confirmed it through testing, and it is not true. New threads copy the values of mask exceptions from the thread that created it, not from the main thread. So your change is safe too.

  21. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    @viniciusfbb let me see where Skia is changing exception masks. By now, let's stand to the adjustments in the DSC components only.

  22. peardox commented on Aug 21, 2022

    @peardox
    Author

    An important concern is what happens when you call python outside P4D e.g. I'm using my hack above like this

        SafeMaskFPUExceptions(True);
        PyEng.ExecStrings(Shim);
        SafeMaskFPUExceptions(False);
    

    I'm guessing I should also wrap e.g.
    MainModule.delphi_style();

    As well.

    MainModule.delphi_train(); can take a few hours to run (and will be using Skia once a second for reporting) - I'll have to give that a go this week

  23. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    That's why exception masks should be changed and restored per invocation.

  24. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    An important concern is what happens when you call python outside P4D e.g. I'm using my hack above like this

        SafeMaskFPUExceptions(True);
        PyEng.ExecStrings(Shim);
        SafeMaskFPUExceptions(False);
    

    I'm guessing I should also wrap e.g. MainModule.delphi_style();

    As well.

    MainModule.delphi_train(); can take a few hours to run (and will be using Skia once a second for reporting) - I'll have to give that a go this week

    Btw, how are you running a blocking task on main thread and using Skia to report it out of a second thread?

  25. peardox commented on Aug 21, 2022

    @peardox
    Author

    What's exacrly what SafeMaskFPUExceptions() does.

  26. viniciusfbb commented on Aug 21, 2022

    @viniciusfbb

    @viniciusfbb let me see where Skia is changing exception masks.

    There are numerous operations that depend on masks in exceptions, which are executed every frame of the app's rendering, so we decided to change it already on load.
    https://github.com/skia4delphi/skia4delphi/blob/f61e4cc05842e5c8c6f7f6ce340ea2df149032c8/Source/Skia.API.pas#L2103

  27. peardox commented on Aug 21, 2022

    @peardox
    Author

    Just starting a new thread for the module call then hand back to Delphi

    Yesterday they finally told us they have their own task manager (but it'll also be setting the mask to bad values I guess)

  28. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    @viniciusfbb let me see where Skia is changing exception masks.

    There are numerous operations that depend on masks in exceptions, which are executed every frame of the app's rendering, so we decided to change it already on load. https://github.com/skia4delphi/skia4delphi/blob/f61e4cc05842e5c8c6f7f6ce340ea2df149032c8/Source/Skia.API.pas#L2103

    I understand. My concern is that it would affect other libraries out of our sight.

  29. lmbelo commented on Aug 21, 2022

    @lmbelo
    Member

    Just starting a new thread for the module call then hand back to Delphi

    Yesterday they finally told us they have their own task manager (but it'll also be setting the mask to bad values I guess)

    You must be careful while using the Python packages out of the main thread.

  30. peardox commented on Aug 21, 2022

    @peardox
    Author

    This seems to work...

      SafeMaskFPUExceptions(True);
      FTask := TTask.Run(
        procedure()
          begin
            TThread.Synchronize(nil,
              procedure()
              begin
                MainModule.delphi_style();
              end
              )
          end
        );
      SafeMaskFPUExceptions(False);
    
    

    It uses a Skia shader while the thread is running showing data reported back from that thread (it's a boring progress display)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Awaiting FeedbackWaiting for feedback from submitter or others.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions