Opened 16 months ago

Closed 15 months ago

Last modified 15 months ago

#7518 closed task (fixed)

Instruction list length in bco->instrs redundant

Reported by: nomeata Owned by:
Priority: normal Milestone: 7.8.1
Component: Runtime System Version: 7.6.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: Related Tickets:

Description

While reading through the BCO-related code I notice that in the first word of the instruction list of a BCO (bco->instrs->payload[0]), the length of the list (as the multiple of 16-bit-words) is stored. This is used in debugging code, e.g. in the disassembler.

However as far as I can tell this is duplicate information: bco->instrs->bytes already contains this information (as multiple of bytes).

Maybe the instruction list was not a proper StgArrWords? object before and this hack was required? If that is the case, I guess it can be removed now, saving a neglectable amount of memory and cleaning the code a bit.

Change History (10)

comment:1 Changed 16 months ago by simonpj

  • Difficulty set to Unknown
  • Owner set to simonmar
  • Summary changed from Instruction list length in bco->instrs redundand to Instruction list length in bco->instrs redundant

Thanks. Simon M is king in that area so I'll assign to him.

Simon

comment:2 Changed 15 months ago by simonmar

  • Status changed from new to infoneeded

Could you point me to the bit of the code you're talking about? I can't find it.

comment:3 Changed 15 months ago by nomeata

  • Status changed from infoneeded to new

Certainly:

Here, the first element of the instruction list is used to find nbcd:
http://hackage.haskell.org/trac/ghc/browser/rts/Disassembler.c#L276

The interpreter ignores the value (unless debugging is on):
http://hackage.haskell.org/trac/ghc/browser/rts/Interpreter.c#L789

And here, in assembleBCO, the number of instruction is prepended to the list of instructions:
http://hackage.haskell.org/trac/ghc/browser/compiler/ghci/ByteCodeAsm.lhs#L156
The assertion in line 161 supports the observation that the information is duplicated and the size of the ByteArray? is correct.

comment:4 Changed 15 months ago by simonmar

  • Milestone set to 7.8.1

Ah, I see. Yes, I think you're right, this field isn't necessary. It is probably there because the size of an StgArrWords used to be stored in units of words, but it was changed to be bytes.

I'll remove it.

comment:5 Changed 15 months ago by marlowsd@…

commit 0c42e301337bdefa94d0c288bb6d689ac33baa4d

Author: Simon Marlow <marlowsd@gmail.com>
Date:   Wed Jan 9 11:51:58 2013 +0000

    remove unnecessary size field in BCO (#7518)

 compiler/ghci/ByteCodeAsm.lhs |    8 ++------
 rts/Interpreter.c             |    4 +---
 2 files changed, 3 insertions(+), 9 deletions(-)

comment:6 Changed 15 months ago by simonmar

  • Status changed from new to merge

comment:7 Changed 15 months ago by simonmar

  • Owner simonmar deleted
  • Status changed from merge to new

comment:8 Changed 15 months ago by simonmar

  • Resolution set to fixed
  • Status changed from new to closed

comment:9 Changed 15 months ago by nomeata

It seems you missed the code in Disassembler.c

comment:10 Changed 15 months ago by marlowsd@…

commit 343548da7274cd15aaeabe72c6b37bce78e9af9c

Author: Simon Marlow <marlowsd@gmail.com>
Date:   Wed Jan 9 14:46:03 2013 +0000

    fix disassembler after removal of size field in bco->instrs  (#7518)

 rts/Disassembler.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Note: See TracTickets for help on using tickets.