Closed Bug 80329 Opened 19 years ago Closed 19 years ago

Table encoder mapping tables should be const

Categories

(Core :: Internationalization, defect)

All
Mac System 8.5
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla0.9.2

People

(Reporter: sfraser_bugs, Assigned: tetsuroy)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: checked-in)

Attachments

(2 files)

The table encoders have a large number of static data tables, which are created 
like this:

static PRUint16 g_ufMappingTable[] = {
#include "8859-2.uf"
};

However, this form does not declare the mapping table data to be const; the 
compiler/linker therefore has to treat it like mutable global data. Making the 
data const allows the linker to place it into a read-only section of the DLL, 
which reduces the amount of memory-map swapping at runtime.

The fix is to declare the data const:

static PRUint16 const g_ufMappingTable[] = {
#include "8859-2.uf"
};

This applies to the hundreds of auto-generated table encoder files, and could be 
a moderate win at runtime.
Blocks: 74803
No longer blocks: 74803
Keywords: perf
Oops, the correct way to make the table const is with:

static const PRUint16 g_ufMappingTable[] = {
#include "8859-2.uf"
};
Reassign to ftang.
Assignee: nhotta → ftang
roy- could you help me about this. I am still swamp by bidi landing.
Assignee: ftang → yokoyama
Sure.  It looks straight forward. 
Status: NEW → ASSIGNED
sfraser: how do I confirm the reduction of runtime memory swapping?
Target Milestone: --- → mozilla0.9.1
Roy: you'd have to use some tool that dumps out DLL sections (code, readonly 
data, global data etc). There are some links to some docs in bug 74803, but I 
don't know if any of those will tell you exactly what you need to know.
Blocks: 74803
sfraser: assuming you have the tool, can you verify once I post the patch?
I don't have such a tool. I'm going by what the docs tell me happens.
sfraser: can you /r= the patch?
wow, there's a lot of it. r=sfraser
brendan: can you /sr= ? thanks
I did rough analyze with 'size' in linux, first is plain
old build and second is with patch #34426:

% size dist/bin/components/libuconv.so
   text    data     bss     dec     hex filename
  75940    7392      24   83356   1459c dist/bin/components/libuconv.so (before)
  77380    5920      24   83324   1457c dist/bin/components/libuconv.so (after)
Whiteboard: Waiting for /sr=
I read it at hyperspeed, and trust that it compiles without warning.  My only
thought is that maybe you want some of the 'const PRUint16 *g_FooTable[] = { ...
}' tables to be arrays of *const pointers* too -- 'const PRUint16 *const
g_FooTable[] = { ... }'.

Try that if it makes sense -- if these arrays' elements are never assigned to
after being statically initialized.  sr=brendan@mozilla.org either way.

/be
nhotta: can you try the patch on Mac?  I just want to make sure we don't 
have errors/warnings.  Thanks
I used Roy's updated code and built on Macintosh. There was no compile errors,
no additional warning either.
Whiteboard: Waiting for /sr= → Waiting for tree open
checked-in.
Whiteboard: Waiting for tree open → checked-in
checked-in
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
TM to 0.9.2 per PDT triage (it's OK to check it in by Friday or after 0.9.1
branch is made).
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Changing QA contact to ftang for now.  Frank, can you verify this within 
development or provide IQA with a test case?
QA Contact: andreasb → ftang
You need to log in before you can comment on or make changes to this bug.