Remove unused XPT features

RESOLVED FIXED in Firefox 47

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(9 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We can remove some XPT code.
(Assignee)

Comment 1

2 years ago
Created attachment 8719682 [details] [diff] [review]
(part 1) - Remove XPT encoding support

Currently XPT can both encode and decode, but encoding has been handled by
Python code since bug 643817, so the encoding support can be removed. This
results in many simplifications. Some notable changes:

- All the XPTHashTable code (including XPTDatapool::offset_map) is no longer
  necessary.

- PrimitiveTest.cpp and SimpleTypeLib.cpp both don't make much sense without
  encoding support, so I removed them.

- A lot of the version code was already unused, e.g. XPT_VERSION_*,
  XPT_TYPELIB_VERSIONS_STRUCT, XPT_TYPELIB_VERSIONS.
  XPT_MAJOR_INCOMPATIBLE_VERSION is the only thing actually used in version
  checks.

- The patch also removes some code that was dead even before encoding removal,
  such as XPT_ParseVersionString().
Attachment #8719682 - Flags: review?(khuey)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
For part 1 we have:

 9 files changed, 107 insertions(+), 1423 deletions(-)
(Assignee)

Comment 3

2 years ago
Created attachment 8719683 [details] [diff] [review]
(part 2) - Remove unused XPT flags
Attachment #8719683 - Flags: review?(khuey)
FWIW, while I will do these reviews, they are quite low priority to me.
(Assignee)

Updated

2 years ago
Blocks: 1249174
(Assignee)

Comment 5

2 years ago
The XPT_ENCODE removal was inspired by part 1 of bug 1249174 -- removing the argnum2 field is easier when we don't have to worry about encoding.
(Assignee)

Comment 6

2 years ago
Created attachment 8720646 [details] [diff] [review]
(part 3) - Remove almost all support for XPT annotations

XPT supports annotations but xpt.py doesn't generate them except for a single
empty annotation (to indicate there are no real annotations). So we can remove
almost all support for them. This also allows XPTString to be removed.
Attachment #8720646 - Flags: review?(khuey)
(Assignee)

Comment 7

2 years ago
Created attachment 8720647 [details] [diff] [review]
(part 4) - Remove unused fields from XPTConstValue
Attachment #8720647 - Flags: review?(khuey)
(Assignee)

Comment 8

2 years ago
Created attachment 8721161 [details] [diff] [review]
(part 5) - Remove XPTDatapool

It can just be inlined into XPTState, which simplifies things.
Attachment #8721161 - Flags: review?(khuey)
(Assignee)

Comment 9

2 years ago
Created attachment 8721162 [details] [diff] [review]
(part 6) - Stack-allocate XPTState

RegisterBuffer() is the only place that creates an XPTState, and it also
destroys it. So the XPTState can be allocated on the stack, which voids the
need for the creation of an XPTArena.
Attachment #8721162 - Flags: review?(khuey)
(Assignee)

Comment 10

2 years ago
Created attachment 8721813 [details] [diff] [review]
(part 7) - Only define XPTArena::name if XPT_ARENA_LOGGING is defined
Attachment #8721813 - Flags: review?(khuey)
(Assignee)

Comment 11

2 years ago
Created attachment 8721814 [details] [diff] [review]
(part 8) - Remove useless XPT freeing code

XPT has some functions and macros for freeing memory. However, they (a) are
only used on error paths, and (b) don't actually free memory -- they just
optionally log the "freeing" -- because piecewise freeing doesn't make sense
with arena allocation.

This patch removes all that unnecessary machinery.
Attachment #8721814 - Flags: review?(khuey)
(Assignee)

Comment 12

2 years ago
Created attachment 8721839 [details] [diff] [review]
(part 9) - Remove XPT arena logging code

I bet nobody has used this code in a decade. I haven't, even though I've been
working on reducing XPT memory usage. Well-targeted diagnostic printfs are much
more useful than these kind of overview stats, in my experience.
Attachment #8721839 - Flags: review?(khuey)
(Assignee)

Comment 13

2 years ago
These 9 patches reduce the number of lines of code in xpcom/typelib/xpt/*.{h,cpp} from 2680 to 1316, and the diffstat overall is as follows:

 13 files changed, 142 insertions(+), 1805 deletions(-)
(Assignee)

Updated

2 years ago
Summary: Removed unused XPT features → Remove unused XPT features
(Assignee)

Comment 14

2 years ago
Created attachment 8722093 [details] [diff] [review]
(part 4) - Remove unused fields from XPTConstValue

(I tweaked a comment.)
Attachment #8722093 - Flags: review?(khuey)
(Assignee)

Updated

2 years ago
Attachment #8720647 - Attachment is obsolete: true
Attachment #8720647 - Flags: review?(khuey)
(Assignee)

Comment 16

2 years ago
Whoops, part 7 didn't get mentioned in comment 15 because I mistakenly put bug 1249174 in the commit message. No matter. See also bug 1249174 comment 15.

Updated

2 years ago
Depends on: 1251298
(Assignee)

Updated

2 years ago
Blocks: 1251458
You need to log in before you can comment on or make changes to this bug.