bugGNU Octave - Bugs: bug #64977, Explore options for the bytecode...

 
 

bug #64977: Explore options for the bytecode interpreter in Octave 9

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Mon 04 Dec 2023 11:15:17 AM UTC
   
 
Category:  Configuration and Build System Severity:  5 - Blocker
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 9.0.90 Operating System:  * Any
Fixed Release:  None Planned Release:  9.1.0 (current stable)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 12 Jan 2024 09:23:31 PM UTC, comment #26: 

The bytecode interpreter has been moved to a new feature branch.  Marking bug as Fixed and closing report.

Rik <rik5>
Group administrator
Fri 15 Dec 2023 04:05:57 PM UTC, comment #25: 

I've pushed the following change to the bytecode-interpreter branch to add the Makefile target `check-tree-evaluator`:
https://hg.savannah.gnu.org/hgweb/octave/rev/b31991b5c361

Followed-up by the following change to run the test suite with the tree evaluator in the CI on GitHub:
https://hg.savannah.gnu.org/hgweb/octave/rev/17f613a4a843

I left the Makefile target `check` alone, meaning that `make check` runs the test suite with whatever evaluator is set as the default (which is also reasonable imho).


Together with the changes by jwe which addressed the actual topic of this issue, I'm marking this as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Thu 14 Dec 2023 01:32:38 AM UTC, comment #24: 


comment #23:

> Do we need a new Category on the Savannah bug tracker for the Bytecode Interpreter?  I'm starting to get confused when I click on a bug and find out that it isn't a problem with Octave's interpreter, but rather a specific problem with the Bytecode Interpreter alone.


It started with separate tags but got merged into a single Interpreter tag some months ago:
https://octave.discourse.group/t/reexamine-savannah-tags/4659/4

Arun Giridhar <arungiridhar>
Group Member
Wed 13 Dec 2023 11:32:28 PM UTC, comment #23: 

Do we need a new Category on the Savannah bug tracker for the Bytecode Interpreter?  I'm starting to get confused when I click on a bug and find out that it isn't a problem with Octave's interpreter, but rather a specific problem with the Bytecode Interpreter alone.

Rik <rik5>
Group administrator
Sun 10 Dec 2023 01:30:06 PM UTC, comment #22: 

Yes, we could add check-tree-evaluator.

It seems it would be best for "make check" to execute both the check-tree-evaluator and check-bytecode-evaluator targets and also perform some kind of comparison at the end.

John W. Eaton <jwe>
Group administrator
Sun 10 Dec 2023 01:07:56 PM UTC, comment #21: 

Having two separate commands to run the tests with and without bytecode interpreter makes it easier to diff the logs of these runs.
Combining the tests into a single target would make that comparison more difficult.

Maybe, we could replace the current target `check-bytecode-evaluator` with a target `check-tree-evaluator` and explicitly disable the bytecode-evaluator for that?

Markus Mützel <mmuetzel>
Group administrator
Sun 10 Dec 2023 01:02:47 PM UTC, comment #20: 

Good question.  I was only thinking that it would make sense to enable the bytecode interpreter by default on that branch.

I could undo the change but it seems odd to me to have the bytecode interpreter disabled by default on that branch or to have to use a different command to test it if it is enabled by default.

Maybe for now on the bytecode interpreter branch, we should arrange to have make check run the tests with both the parse tree and bytecode interpreters?  If you agree that makes sense, I could take a look at doing that.

John W. Eaton <jwe>
Group administrator
Sun 10 Dec 2023 12:51:14 PM UTC, comment #19: 

With that changed, is there a difference between running `make check` and `make check-bytecode-evaluator`?
How to best check if changes on the feature branch are breaking the parse-tree evaluator?

Markus Mützel <mmuetzel>
Group administrator
Sun 10 Dec 2023 12:42:34 PM UTC, comment #18: 

Since it is the bytecode interpreter branch, I have also now changed the default setting to enable using the bytecode interpreter by default on that branch in this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/58c1f4c52e83

John W. Eaton <jwe>
Group administrator
Sun 10 Dec 2023 11:42:10 AM UTC, comment #17: 

Compiling the bytecode evaluator is enabled by default on the new feature branch (just like it was a couple of weeks ago on the default branch). To disable building it, you'd need to configure with `--disable-bytecode-evaluator`.

Markus Mützel <mmuetzel>
Group administrator
Sun 10 Dec 2023 04:05:56 AM UTC, comment #16: 

Is the byte code enabled on the bytecode branch, by default?
If not I think it should be.
What is the command to enable it?

Doug Stewart <dastew>
Sat 09 Dec 2023 04:51:57 PM UTC, comment #15: 

It seems quite fine with a feature branch. There is not too much code changes in the relevant parts of the code base anyway. Could I maybe get write access to the branch? I think that would simplify.

comment #6:

> RE: the patch in comment #4, why do these functions need to be tagged with these attributes?  I'd rather avoid the clutter unless it is necessary.  What benefit do these attributes provide for these particular functions?


The whole octave_value_vm class is more or less some shenanigans to fool the gcc optimizer into optimizing in a special way for some different problems. The main problem is to not call ~octave_value () when m_rep is nullptr due to a move. C++ optimizations seems quite brittle in general and are hard to reason about, so a 'wrapper class' is quite convenient way to force behaviour. The class can probably be refactored out though, because I think the optimizer got better since gcc 10, and by using octave_base_value* instead of a octave_value some of the problems could be mitigated, I discovered.

Petter <petter>
Fri 08 Dec 2023 10:40:22 PM UTC, comment #14: 

I originally agreed to put the bytecode interpreter on the default branch because I thought that if more people were building it and using it, maybe more people would look at it and do some review.  I also thought that if there was any chance for it to ultimately become part of Octave then it would be helpful to have it merged early rather than waiting until it was larger and probably more complex, making review even more difficult.

In the 5 months since the code was first merged on the default branch we have obviously seen that some people are trying to use it.  And although compiling the bytecode interpreter and checking that your Octave code still works when it is enabled is helpful, it is not a substitute for actual code review.

Since there hasn't been much significant code review in the last 5 months, it seems to me that moving it back to the default branch now won't increase the rate of the code review process.

My current assessment is that it is not ready for a stable release and so it won't be merged back to the default branch until there has been sufficient review and revision.

As I mentioned earlier, by keeping it on a feature branch it is easy to know exactly what changes are part of the bytecode interpreter.  That makes it easy to know what changes need to be reviewed.

No one is taking anything away.  The code is still available in the Octave source repository and anyone is free to update to and build from the bytecode-interpreter branch, examine the code, offer review, and help push the project forward.

Finally, we are all volunteers.  We don't do timelines, milestones, or assign roles and responsibilities any more than someone wishes to volunteer their time to work on a specific project and set goals for themselves.

John W. Eaton <jwe>
Group administrator
Fri 08 Dec 2023 06:59:03 PM UTC, comment #13: 

I'm not sure what other version number should be used.

Unless you are actively working on the bytecode interpreter, I don't think you need to worry about merging changes there.  It seems to me that it is the responsibility of the people working on the feature branch to keep it up to date with default.

There is no specific frequency for merging either stable to default or default to a feature branch.  I'd say at least often enough to avoid significant merge conflicts or as often as you'd like so that changes on stable and default appear reasonably soon after on the default and feature branch.

John W. Eaton <jwe>
Group administrator
Fri 08 Dec 2023 06:49:10 PM UTC, comment #12: 

in addition to Doug's question, i noticed after the last change to stable a merge stable to default, then a merge default to bytecode-interpreter was performed. how regular should that second step be for changes made to stable and default?

Nicholas Jankowski <nrjank>
Group Member
Fri 08 Dec 2023 04:46:22 PM UTC, comment #11: 

I compiled the new bytecode version, and it reports

GNU Octave, version 10.0.0
 version -hgid
ans = a7b2d35d56e4

should it have a different number than 10.0.0 ?

Doug Stewart <dastew>
Fri 08 Dec 2023 10:59:45 AM UTC, comment #10: 

Coming back to the "valid and agreed plan including a list of (fullfillable and realistic) acceptance criteria of the VM into default/stable/a release".
A plan contains tasks, timeline, milestones and especially roles & responsibilities: who does what. And this is a usually a problem in an open source development environment like Octave.

Putting anything complex and core like the bytecode interpreter live will always have residual risks, so what will change in a year or two ?
Especially as development, test + review will need to be done outside the mainstream activities/main branch, so by default with limited focus (if any) from the broader community.

Such a plan or an explicit or implicit commitment from some of the core team may already or will exist. I do not know.
But without it I think that there are good chances that the VM will not see its way (back) into default let alone into stable or a release in a reasonable future.

Hendrik K <koerhen>
Fri 08 Dec 2023 09:53:34 AM UTC, comment #9: 

I don't think that the development of the bytecode interpreter will be slowed down due to it being moved to a feature branch. In contrast, the exemption from proper review that we de-facto used for a while for patches concerning the bytecode interpreter might be continued for a while on that feature branch. (But we haven't talked about that. So, don't quote me on that.)

We definitely couldn't have continued with that exemption for the indefinite future. So, keeping its development on the default branch would probably have actually led to a slow-down of its development.

Markus Mützel <mmuetzel>
Group administrator
Fri 08 Dec 2023 02:28:44 AM UTC, comment #8: 

I share your concern that moving it to a feature branch, which we do not often use in Octave's development process, puts it at risk of developing slower if for no other reason than it won't be there to 'get in the way' as people work on other things. Unfortunately, as we mainly only work with one development branch, that simpler approach means that on a major release everything from that dev branch will get merged into the stable branch whether it's ready or not.  That's what happened here with the VM / bytecode interpreter.  Out of concern that it still may not be ready for stable with Octave 10, the feature branch was the best method of keeping it's code separate.

Maybe with enough focused effort that will change over the next year and it will get merged back into the development branch before octave 10. maybe not. but there are a number of people who work with a 'bleeding edge' version of octave compiled from the development source.   perhaps we could dedicate some resources to regularly building a version of the bytecode-interpreter branch for some platforms. I think anyone with resources can set up a bulidbot client to do so.

Nicholas Jankowski <nrjank>
Group Member
Fri 08 Dec 2023 02:16:15 AM UTC, comment #7: 

I think that the VM is one of most value-adding enhancement of Octave in the past years for all users who develop often used and CPU intensive and time consuming projects.

I observed material performance enhancement of 60%+ in complex, already highly vectorized and performance tuned octave applications. Of course if one does mainly rapid prototyping with octave, performance does not count much: So the VM is of less advantage for such a user group.


Except if there is a valid and agreed plan including a list of (fullfillable and realistic) acceptance criteria of the VM into default/stable/a release, the current approach taken to move it to a separate branch has very good chances that the VM will never see its way (back) into default let alone into stable or a release.

(Who from the octave community compiles octave on its own and on top will work on a separate non default-branch ?)

Anonymous
Thu 07 Dec 2023 08:27:33 PM UTC, comment #6: 

RE: the patch in comment #4, why do these functions need to be tagged with these attributes?  I'd rather avoid the clutter unless it is necessary.  What benefit do these attributes provide for these particular functions?

John W. Eaton <jwe>
Group administrator
Thu 07 Dec 2023 07:55:31 AM UTC, comment #5: 

I pushed a series of changes to move the bytecode interpreter to a separate feature branch (named bytecode-interpreter).

We can still discuss whether it is better to have a separate feature branch and maybe after that discussion we'll abandon it.  Eliminating it would be as simple as merging the new branch back to default and closing it.

However, since I had already done the work to remove the bytecode interpreter from the stable branch it seemed to me that it would be easier to move it to a separate branch at the same time before any further changes were made to it on the default branch. 

Having a separate branch makes it very easy to see the complete set of changes that make up the bytecode interpreter by doing


hg diff -r default -r bytecode-interpreter


immediately after a merge of default to the bytecode-interpreter branch.

So from now until we decide to merge the bytecode interpreter with default, please make all changes related to the bytecode interpreter on the bytecode-interpreter branch and merge default to that branch often to stay current with changes made on default.

John W. Eaton <jwe>
Group administrator
Mon 04 Dec 2023 10:49:57 PM UTC, comment #4: 

The incompatibility risk I am aware of is the computed goto:s in execute_code () in pt-bytecode-vm.cc. I believe if compilation issues are the concern, #if guards for the whole body of execute_code () should make compilers without that GNU extension happy and if-guards around user facing functions that makes no sense without being able to run the bytecode interpreter.

The bytecode interpreter is tested on 32-bit systems without problem, and there is code to support big endian systems, but I have had no way to try that on an actual big endian system.

I compiled with CXXFLAGS="-pedantic -std=c++17" and the complaints were:

./../libinterp/parse-tree/oct-parse.yy:417:53: warning: ISO C++11 requires at least one argument for the "..." in a variadic macro
  417 |                     OCTAVE_YYUSE ($2);


./../liboctave/system/file-ops.cc: In function 'std::string octave::sys::tempnam(const std::string&, const std::string&, std::string&)':
./../liboctave/system/file-ops.cc:752:8: warning: ISO C++ forbids variable length array 'tname' [-Wvla]
  752 |   char tname [templatename.length () + 1];
|        ^~~~~

./../liboctave/util/oct-string.cc: In member function 'virtual int octave::string::codecvt_u8::do_length(StateT&, const ExternT*, const ExternT*, std::size_t) const':
./../liboctave/util/oct-string.cc:894:15: warning: ISO C++ forbids variable length array 'offsets' [-Wvla]
  894 |   std::size_t offsets[srclen];
      |               ^~~~~~~


'pedantic' didn't trigger on the computed goto:s, so it does not seem to warn on every GNU extension.

However I did find some function attributes I've missed guarding. I would suggest something like the attached patch for those:

(file #55404)

Petter <petter>
Mon 04 Dec 2023 06:29:08 PM UTC, comment #3: 

My preference is that we should do at least option 1.

I'd really prefer at least option 2 plus keeping the optional compilation on the default branch.  I don't think this feature belongs on stable and in a release yet, even if compilation is disabled by default.  I'll take a look at making those changes next.

If we think that there is a good possibility that the bytecode interpreter won't be ready for Octave 10 either, then we should probably move it to a feature branch so we can avoid facing this problem again in another year.

I'm attaching the changes I've made for option 1.  When I'll push them to stable and merge with default when savannah is working for me again.

The first change restores the --disable-bytecode-evaluator configure option.

The second skips compilation of bytecode evaluator code unless OCTAVE_ENABLE_BYTECODE_EVALUATOR is defined and also makes it so that OCTAVE_ENABLE_BYTECODE_EVALUATOR is not defined by default.

When I merge to default, I'll omit the changes to configure.ac from the second changeset and arrange so that the bytecode evaluator is still compiled by default on the default branch.

Note that even on the stable branch with the bytecode evaluator disabled, functions like _vm_enable_  are defined but they produce errors like


error: __vm_enable__: support for byte-compiled functions was unavailable or disabled when Octave was built


because doing that was easier than dealing with the documentation system.  Likewise, I inserted guards in all the affected files instead of removing them from the lists of files to compile because it was easier than trying to figure out how to skip compilation of the files and still ensure that they are included in tar files created by "make dist".

Also, if we had used a feature branch for these changes then this wouldn't be an issue for the release because the changes would only be on the feature branch and so would not have appeared on stable when we merged default to stable for the release.


(file #55401, file #55402)

John W. Eaton <jwe>
Group administrator
Mon 04 Dec 2023 05:45:49 PM UTC, comment #2: 

I don't think we can do option 1 in a dot release (i.e., after the release of Octave 9 and packagers potentially reporting issues). It would likely affect the API which distributors don't like to happen in a dot-release.
We'd probably need to either try and fix those potential issues (in that experimental and so far mostly unreviewed part of the code) or follow up with Octave 10.

Imho, option 0 is the most risky one.

Markus Mützel <mmuetzel>
Group administrator
Mon 04 Dec 2023 05:37:31 PM UTC, comment #1: 

Would prefer 0 (if possible) and 1 (if necessary). E.g., if a distro cannot package Octave 9 because Octave 9 doesn't build on an uncommon platform they support, that might make 1 necessary. If there aren't such reports, 0 might be OK.

Arun Giridhar <arungiridhar>
Group Member
Mon 04 Dec 2023 11:15:17 AM UTC, original submission:  

During the developer meeting last week, we talked about different options for how to handle the code for the bytecode interpreter (which is on the stable branch now) for Octave 9.
We are not sure if all of the new code is platform and compiler agnostic. So, there is the risk that it could fail to compile on some (obscure) platform or compiler combinations.

To still allow users on potentially affected platforms to build Octave without the bytecode interpreter, we talked about the following options:

0. Leave as-is. Don't do anything with respect to the bytecode interpreter on the stable branch before Octave 9.

1. Re-instate the configuration option that allows to skip compiling the parts of the code that are required only for the bytecode interpreter. That would also require to identify those portions of the code and guard them with preprocessor guards or potentially not building some object files entirely.

2. Revert the changes for the bytecode interpreter on the stable branch and keep the changes only on the default branch. That would also require to identify the respective parts. But compared to option 1, they would be removed on the stable branch (but not the default branch) instead of adding preprocessor guards around them.

3. Revert the changes for the bytecode interpreter on both the stable and the default branches and move its development to a feature branch that could be merged when the bytecode interpreter is deemed to be ready. This would entail similar edits as for option 2. But they would affect the stable and the default branches.

There might have been more options that I don't recall.

My personal favorite would be option 1.

What do other developers think? Any favorites or alternative options?

Markus Mützel <mmuetzel>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #55404:  32841.patch added by petter (2KiB - text/x-patch)
file #55401:  bytecode-disable-1.txt added by jwe (4KiB - text/plain)
file #55402:  bytecode-disable-2.txt added by jwe (71KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by dastew (Posted a comment)
  • -email is unavailable- added by koerhen (Posted a comment)
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by petter (Updated the item)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by mmuetzel
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-01-12 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-12-15 mmuetzel StatusNone Ready For Test
    2023-12-04 petter Attached File- Added 32841.patch, #55404
    2023-12-04 jwe Attached File- Added bytecode-disable-1.txt, #55401
        Attached File- Added bytecode-disable-2.txt, #55402
    2023-12-04 mmuetzel Carbon-Copy- Added rik5
        Carbon-Copy- Added jwe
        Carbon-Copy- Added petter

    Back to the top

    Powered by Savane 3.12.
    Corresponding source code